From 3bfe94d202600e5550e7659baaa2549bfeedd01d Mon Sep 17 00:00:00 2001 From: glank Date: Wed, 20 Aug 2025 17:04:18 +0200 Subject: [PATCH 1/4] Fix coloring --- cli/cmdlineparser.cpp | 43 ++++++++++++++++++++++++++++++--- cli/cppcheckexecutor.cpp | 19 ++++++++++++--- lib/color.cpp | 35 ++++----------------------- lib/color.h | 5 ++-- lib/errorlogger.cpp | 51 ++++++++++++++++++++++++++-------------- lib/errorlogger.h | 4 ++-- test/main.cpp | 1 - test/testcolor.cpp | 3 +-- 8 files changed, 100 insertions(+), 61 deletions(-) diff --git a/cli/cmdlineparser.cpp b/cli/cmdlineparser.cpp index 7010e5c3f5e..e6bc790b231 100644 --- a/cli/cmdlineparser.cpp +++ b/cli/cmdlineparser.cpp @@ -56,6 +56,14 @@ #include #include +#ifdef _WIN32 +#include +#include +#else +#include +#include +#endif + #ifdef HAVE_RULES // xml is used for rules #include "xml.h" @@ -1553,9 +1561,38 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a if (mSettings.templateLocation.empty()) mSettings.templateLocation = "{bold}{file}:{line}:{column}: {dim}note:{reset} {info}\\n{code}"; } - // replace static parts of the templates - substituteTemplateFormatStatic(mSettings.templateFormat); - substituteTemplateLocationStatic(mSettings.templateLocation); + + { + bool enableColors = false; + + if (!getForcedColorSetting(enableColors) && mSettings.outputFormat == Settings::OutputFormat::text) { +#ifdef _WIN32 + if (mSettings.outputFile.empty()) + enableColors = _isatty(_fileno(stderr)); + else { + int fd = _open(mSettings.outputFile.c_str(), _O_RDONLY); + if (fd != -1) { + enableColors = (_isatty(fd) != 0); + _close(fd); + } + } +#else + if (mSettings.outputFile.empty()) + enableColors = isatty(fileno(stderr)); + else { + int fd = open(mSettings.outputFile.c_str(), O_RDONLY | O_NOCTTY); + if (fd != -1) { + enableColors = (isatty(fd) != 0); + close(fd); + } + } +#endif + } + + // replace static parts of the templates + substituteTemplateFormatStatic(mSettings.templateFormat, enableColors); + substituteTemplateLocationStatic(mSettings.templateLocation, enableColors); + } if (mSettings.force || maxconfigs) mSettings.checkAllConfigurations = true; diff --git a/cli/cppcheckexecutor.cpp b/cli/cppcheckexecutor.cpp index 184cd00cd2f..a3936b2e335 100644 --- a/cli/cppcheckexecutor.cpp +++ b/cli/cppcheckexecutor.cpp @@ -70,7 +70,10 @@ #endif #ifdef _WIN32 +#include #include +#else +#include #endif #if !defined(WIN32) && !defined(__MINGW32__) @@ -612,10 +615,20 @@ void StdLogger::reportErr(const std::string &errmsg) void StdLogger::reportOut(const std::string &outmsg, Color c) { - if (c == Color::Reset) - std::cout << ansiToOEM(outmsg, true) << std::endl; - else + bool enableColors; + + if (!getForcedColorSetting(enableColors)) { +#ifdef _WIN32 + enableColors = (_isatty(_fileno(stdout)) != 0); +#else + enableColors = (isatty(fileno(stdout)) != 0); +#endif + } + + if (enableColors && c != Color::Reset) std::cout << c << ansiToOEM(outmsg, true) << Color::Reset << std::endl; + else + std::cout << ansiToOEM(outmsg, true) << std::endl; } // TODO: remove filename parameter? diff --git a/lib/color.cpp b/lib/color.cpp index 2a29fb61376..4a8bbb12a6c 100644 --- a/lib/color.cpp +++ b/lib/color.cpp @@ -22,52 +22,27 @@ #include #include -#ifndef _WIN32 -#include -#endif - -bool gDisableColors = false; - -#ifndef _WIN32 -static bool isStreamATty(const std::ostream & os) -{ - static const bool stdout_tty = isatty(STDOUT_FILENO); - static const bool stderr_tty = isatty(STDERR_FILENO); - if (&os == &std::cout) - return stdout_tty; - if (&os == &std::cerr) - return stderr_tty; - return (stdout_tty && stderr_tty); -} -#endif - -static bool isColorEnabled(const std::ostream & os) +bool getForcedColorSetting(bool &colorSetting) { // See https://bixense.com/clicolors/ static const bool color_forced_off = (nullptr != std::getenv("NO_COLOR")); if (color_forced_off) { - return false; + colorSetting = false; + return true; } static const bool color_forced_on = (nullptr != std::getenv("CLICOLOR_FORCE")); if (color_forced_on) { + colorSetting = true; return true; } -#ifdef _WIN32 - (void)os; return false; -#else - return isStreamATty(os); -#endif } std::ostream& operator<<(std::ostream & os, Color c) { - if (!gDisableColors && isColorEnabled(os)) - return os << "\033[" << static_cast(c) << "m"; - - return os; + return os << "\033[" << static_cast(c) << "m"; } std::string toString(Color c) diff --git a/lib/color.h b/lib/color.h index 1a677e8393a..0c3d0265849 100644 --- a/lib/color.h +++ b/lib/color.h @@ -35,10 +35,11 @@ enum class Color : std::uint8_t { FgMagenta = 35, FgDefault = 39 }; + +CPPCHECKLIB bool getForcedColorSetting(bool &colorSetting); + CPPCHECKLIB std::ostream& operator<<(std::ostream& os, Color c); CPPCHECKLIB std::string toString(Color c); -extern CPPCHECKLIB bool gDisableColors; // for testing - #endif diff --git a/lib/errorlogger.cpp b/lib/errorlogger.cpp index 183d130025a..8466c437f50 100644 --- a/lib/errorlogger.cpp +++ b/lib/errorlogger.cpp @@ -594,20 +594,35 @@ static void replace(std::string& source, const std::unordered_map substitutionMap = - { - {"{reset}", ::toString(Color::Reset)}, - {"{bold}", ::toString(Color::Bold)}, - {"{dim}", ::toString(Color::Dim)}, - {"{red}", ::toString(Color::FgRed)}, - {"{green}", ::toString(Color::FgGreen)}, - {"{blue}", ::toString(Color::FgBlue)}, - {"{magenta}", ::toString(Color::FgMagenta)}, - {"{default}", ::toString(Color::FgDefault)}, - }; - replace(source, substitutionMap); +static void replaceColors(std::string& source, bool enableColors) { + if (enableColors) { + static const std::unordered_map substitutionMap = + { + {"{reset}", ::toString(Color::Reset)}, + {"{bold}", ::toString(Color::Bold)}, + {"{dim}", ::toString(Color::Dim)}, + {"{red}", ::toString(Color::FgRed)}, + {"{green}", ::toString(Color::FgGreen)}, + {"{blue}", ::toString(Color::FgBlue)}, + {"{magenta}", ::toString(Color::FgMagenta)}, + {"{default}", ::toString(Color::FgDefault)}, + }; + replace(source, substitutionMap); + } + else { + static const std::unordered_map substitutionMap = + { + {"{reset}", ""}, + {"{bold}", ""}, + {"{dim}", ""}, + {"{red}", ""}, + {"{green}", ""}, + {"{blue}", ""}, + {"{magenta}", ""}, + {"{default}", ""}, + }; + replace(source, substitutionMap); + } } std::string ErrorMessage::toString(bool verbose, const std::string &templateFormat, const std::string &templateLocation) const @@ -913,16 +928,16 @@ std::string replaceStr(std::string s, const std::string &from, const std::string return s; } -void substituteTemplateFormatStatic(std::string& templateFormat) +void substituteTemplateFormatStatic(std::string& templateFormat, bool enableColors) { replaceSpecialChars(templateFormat); - replaceColors(templateFormat); + replaceColors(templateFormat, enableColors); } -void substituteTemplateLocationStatic(std::string& templateLocation) +void substituteTemplateLocationStatic(std::string& templateLocation, bool enableColors) { replaceSpecialChars(templateLocation); - replaceColors(templateLocation); + replaceColors(templateLocation, enableColors); } std::string getClassification(const std::string &guideline, ReportType reportType) { diff --git a/lib/errorlogger.h b/lib/errorlogger.h index 44e3b733cdc..9e7feb6c120 100644 --- a/lib/errorlogger.h +++ b/lib/errorlogger.h @@ -288,10 +288,10 @@ class CPPCHECKLIB ErrorLogger { std::string replaceStr(std::string s, const std::string &from, const std::string &to); /** replaces the static parts of the location template **/ -CPPCHECKLIB void substituteTemplateFormatStatic(std::string& templateFormat); +CPPCHECKLIB void substituteTemplateFormatStatic(std::string& templateFormat, bool enableColors = false); /** replaces the static parts of the location template **/ -CPPCHECKLIB void substituteTemplateLocationStatic(std::string& templateLocation); +CPPCHECKLIB void substituteTemplateLocationStatic(std::string& templateLocation, bool enableColors = false); /** Get a classification string from the given guideline and reporttype */ CPPCHECKLIB std::string getClassification(const std::string &guideline, ReportType reportType); diff --git a/test/main.cpp b/test/main.cpp index 571b126922d..5ec90ed43df 100644 --- a/test/main.cpp +++ b/test/main.cpp @@ -31,7 +31,6 @@ int main(int argc, char *argv[]) #endif Preprocessor::macroChar = '$'; // While macroChar is char(1) per default outside test suite, we require it to be a human-readable character here. - gDisableColors = true; options args(argc, argv); diff --git a/test/testcolor.cpp b/test/testcolor.cpp index 7351cb2df9e..c8086c89fea 100644 --- a/test/testcolor.cpp +++ b/test/testcolor.cpp @@ -30,8 +30,7 @@ class TestColor : public TestFixture { } void toString() const { - // TODO: color conversion is dependent on stdout/stderr being a TTY - ASSERT_EQUALS("", ::toString(Color::FgRed)); + ASSERT_EQUALS("\033[31m", ::toString(Color::FgRed)); } }; From e00a762f27fbe707486833808314fdd4861e7468 Mon Sep 17 00:00:00 2001 From: glank Date: Wed, 20 Aug 2025 18:30:26 +0200 Subject: [PATCH 2/4] Add test --- test/cli/other_test.py | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/test/cli/other_test.py b/test/cli/other_test.py index f50f8d1f74c..51e8b6bd3d9 100644 --- a/test/cli/other_test.py +++ b/test/cli/other_test.py @@ -2426,6 +2426,41 @@ def test_xml_output(tmp_path): # #13391 / #13485 '''.format(version_str, test_file_exp, test_file_exp, test_file_exp)) +def test_outputfile(tmp_path): # #14051 + test_file = tmp_path / 'test.cpp' + out_file = tmp_path / 'out.txt' + with open(test_file, 'wt') as f: + f.write( +""" +int main() +{ + int x = 1 / 0; +} +""") + + args = [ + '-q', + '--output-file={}'.format(out_file), + str(test_file) + ] + + out_exp = [ + '{}:4:15: error: Division by zero. [zerodiv]'.format(test_file), + ' int x = 1 / 0;', + ' ^', + ] + + exitcode, stdout, stderr = cppcheck(args) + assert exitcode == 0, stdout + assert stdout == '' + assert stderr == '' + + with open(out_file, 'rt') as f: + out_text = f.read(); + + assert out_text.splitlines() == out_exp + + def test_internal_error_loc_int(tmp_path): test_file = tmp_path / 'test.c' with open(test_file, 'wt') as f: From ed254554de6d9de9826b93b5bdf06bf208c8ea0e Mon Sep 17 00:00:00 2001 From: glank Date: Wed, 20 Aug 2025 19:41:36 +0200 Subject: [PATCH 3/4] Fix pylint issue --- test/cli/other_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cli/other_test.py b/test/cli/other_test.py index 51e8b6bd3d9..ec73fef11e3 100644 --- a/test/cli/other_test.py +++ b/test/cli/other_test.py @@ -2456,7 +2456,7 @@ def test_outputfile(tmp_path): # #14051 assert stderr == '' with open(out_file, 'rt') as f: - out_text = f.read(); + out_text = f.read() assert out_text.splitlines() == out_exp From 6dcf71c8778be0609a9aaf780b34799fb9086507 Mon Sep 17 00:00:00 2001 From: glank Date: Fri, 22 Aug 2025 12:42:04 +0200 Subject: [PATCH 4/4] Quick fix --- cli/cmdlineparser.cpp | 43 ++------------------------ cli/cppcheckexecutor.cpp | 19 ++---------- lib/color.cpp | 35 ++++++++++++++++++---- lib/color.h | 5 ++-- lib/errorlogger.cpp | 65 ++++++++++++++++++++-------------------- lib/errorlogger.h | 4 +-- test/main.cpp | 1 + test/testcolor.cpp | 3 +- 8 files changed, 75 insertions(+), 100 deletions(-) diff --git a/cli/cmdlineparser.cpp b/cli/cmdlineparser.cpp index e6bc790b231..bbd88ac0c5c 100644 --- a/cli/cmdlineparser.cpp +++ b/cli/cmdlineparser.cpp @@ -56,14 +56,6 @@ #include #include -#ifdef _WIN32 -#include -#include -#else -#include -#include -#endif - #ifdef HAVE_RULES // xml is used for rules #include "xml.h" @@ -1561,38 +1553,9 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a if (mSettings.templateLocation.empty()) mSettings.templateLocation = "{bold}{file}:{line}:{column}: {dim}note:{reset} {info}\\n{code}"; } - - { - bool enableColors = false; - - if (!getForcedColorSetting(enableColors) && mSettings.outputFormat == Settings::OutputFormat::text) { -#ifdef _WIN32 - if (mSettings.outputFile.empty()) - enableColors = _isatty(_fileno(stderr)); - else { - int fd = _open(mSettings.outputFile.c_str(), _O_RDONLY); - if (fd != -1) { - enableColors = (_isatty(fd) != 0); - _close(fd); - } - } -#else - if (mSettings.outputFile.empty()) - enableColors = isatty(fileno(stderr)); - else { - int fd = open(mSettings.outputFile.c_str(), O_RDONLY | O_NOCTTY); - if (fd != -1) { - enableColors = (isatty(fd) != 0); - close(fd); - } - } -#endif - } - - // replace static parts of the templates - substituteTemplateFormatStatic(mSettings.templateFormat, enableColors); - substituteTemplateLocationStatic(mSettings.templateLocation, enableColors); - } + // replace static parts of the templates + substituteTemplateFormatStatic(mSettings.templateFormat, !mSettings.outputFile.empty()); + substituteTemplateLocationStatic(mSettings.templateLocation, !mSettings.outputFile.empty()); if (mSettings.force || maxconfigs) mSettings.checkAllConfigurations = true; diff --git a/cli/cppcheckexecutor.cpp b/cli/cppcheckexecutor.cpp index a3936b2e335..184cd00cd2f 100644 --- a/cli/cppcheckexecutor.cpp +++ b/cli/cppcheckexecutor.cpp @@ -70,10 +70,7 @@ #endif #ifdef _WIN32 -#include #include -#else -#include #endif #if !defined(WIN32) && !defined(__MINGW32__) @@ -615,20 +612,10 @@ void StdLogger::reportErr(const std::string &errmsg) void StdLogger::reportOut(const std::string &outmsg, Color c) { - bool enableColors; - - if (!getForcedColorSetting(enableColors)) { -#ifdef _WIN32 - enableColors = (_isatty(_fileno(stdout)) != 0); -#else - enableColors = (isatty(fileno(stdout)) != 0); -#endif - } - - if (enableColors && c != Color::Reset) - std::cout << c << ansiToOEM(outmsg, true) << Color::Reset << std::endl; - else + if (c == Color::Reset) std::cout << ansiToOEM(outmsg, true) << std::endl; + else + std::cout << c << ansiToOEM(outmsg, true) << Color::Reset << std::endl; } // TODO: remove filename parameter? diff --git a/lib/color.cpp b/lib/color.cpp index 4a8bbb12a6c..2a29fb61376 100644 --- a/lib/color.cpp +++ b/lib/color.cpp @@ -22,27 +22,52 @@ #include #include -bool getForcedColorSetting(bool &colorSetting) +#ifndef _WIN32 +#include +#endif + +bool gDisableColors = false; + +#ifndef _WIN32 +static bool isStreamATty(const std::ostream & os) +{ + static const bool stdout_tty = isatty(STDOUT_FILENO); + static const bool stderr_tty = isatty(STDERR_FILENO); + if (&os == &std::cout) + return stdout_tty; + if (&os == &std::cerr) + return stderr_tty; + return (stdout_tty && stderr_tty); +} +#endif + +static bool isColorEnabled(const std::ostream & os) { // See https://bixense.com/clicolors/ static const bool color_forced_off = (nullptr != std::getenv("NO_COLOR")); if (color_forced_off) { - colorSetting = false; - return true; + return false; } static const bool color_forced_on = (nullptr != std::getenv("CLICOLOR_FORCE")); if (color_forced_on) { - colorSetting = true; return true; } +#ifdef _WIN32 + (void)os; return false; +#else + return isStreamATty(os); +#endif } std::ostream& operator<<(std::ostream & os, Color c) { - return os << "\033[" << static_cast(c) << "m"; + if (!gDisableColors && isColorEnabled(os)) + return os << "\033[" << static_cast(c) << "m"; + + return os; } std::string toString(Color c) diff --git a/lib/color.h b/lib/color.h index 0c3d0265849..1a677e8393a 100644 --- a/lib/color.h +++ b/lib/color.h @@ -35,11 +35,10 @@ enum class Color : std::uint8_t { FgMagenta = 35, FgDefault = 39 }; - -CPPCHECKLIB bool getForcedColorSetting(bool &colorSetting); - CPPCHECKLIB std::ostream& operator<<(std::ostream& os, Color c); CPPCHECKLIB std::string toString(Color c); +extern CPPCHECKLIB bool gDisableColors; // for testing + #endif diff --git a/lib/errorlogger.cpp b/lib/errorlogger.cpp index 8466c437f50..afc5a02ca5e 100644 --- a/lib/errorlogger.cpp +++ b/lib/errorlogger.cpp @@ -594,35 +594,34 @@ static void replace(std::string& source, const std::unordered_map substitutionMap = - { - {"{reset}", ::toString(Color::Reset)}, - {"{bold}", ::toString(Color::Bold)}, - {"{dim}", ::toString(Color::Dim)}, - {"{red}", ::toString(Color::FgRed)}, - {"{green}", ::toString(Color::FgGreen)}, - {"{blue}", ::toString(Color::FgBlue)}, - {"{magenta}", ::toString(Color::FgMagenta)}, - {"{default}", ::toString(Color::FgDefault)}, - }; - replace(source, substitutionMap); - } - else { - static const std::unordered_map substitutionMap = - { - {"{reset}", ""}, - {"{bold}", ""}, - {"{dim}", ""}, - {"{red}", ""}, - {"{green}", ""}, - {"{blue}", ""}, - {"{magenta}", ""}, - {"{default}", ""}, - }; - replace(source, substitutionMap); - } +static void replaceColors(std::string& source, bool erase) { + // TODO: colors are not applied when either stdout or stderr is not a TTY because we resolve them before the stream usage + static const std::unordered_map substitutionMapReplace = + { + {"{reset}", ::toString(Color::Reset)}, + {"{bold}", ::toString(Color::Bold)}, + {"{dim}", ::toString(Color::Dim)}, + {"{red}", ::toString(Color::FgRed)}, + {"{green}", ::toString(Color::FgGreen)}, + {"{blue}", ::toString(Color::FgBlue)}, + {"{magenta}", ::toString(Color::FgMagenta)}, + {"{default}", ::toString(Color::FgDefault)}, + }; + static const std::unordered_map substitutionMapErase = + { + {"{reset}", ""}, + {"{bold}", ""}, + {"{dim}", ""}, + {"{red}", ""}, + {"{green}", ""}, + {"{blue}", ""}, + {"{magenta}", ""}, + {"{default}", ""}, + }; + if (!erase) + replace(source, substitutionMapReplace); + else + replace(source, substitutionMapErase); } std::string ErrorMessage::toString(bool verbose, const std::string &templateFormat, const std::string &templateLocation) const @@ -928,16 +927,16 @@ std::string replaceStr(std::string s, const std::string &from, const std::string return s; } -void substituteTemplateFormatStatic(std::string& templateFormat, bool enableColors) +void substituteTemplateFormatStatic(std::string& templateFormat, bool eraseColors) { replaceSpecialChars(templateFormat); - replaceColors(templateFormat, enableColors); + replaceColors(templateFormat, eraseColors); } -void substituteTemplateLocationStatic(std::string& templateLocation, bool enableColors) +void substituteTemplateLocationStatic(std::string& templateLocation, bool eraseColors) { replaceSpecialChars(templateLocation); - replaceColors(templateLocation, enableColors); + replaceColors(templateLocation, eraseColors); } std::string getClassification(const std::string &guideline, ReportType reportType) { diff --git a/lib/errorlogger.h b/lib/errorlogger.h index 9e7feb6c120..5101f765fad 100644 --- a/lib/errorlogger.h +++ b/lib/errorlogger.h @@ -288,10 +288,10 @@ class CPPCHECKLIB ErrorLogger { std::string replaceStr(std::string s, const std::string &from, const std::string &to); /** replaces the static parts of the location template **/ -CPPCHECKLIB void substituteTemplateFormatStatic(std::string& templateFormat, bool enableColors = false); +CPPCHECKLIB void substituteTemplateFormatStatic(std::string& templateFormat, bool eraseColors = false); /** replaces the static parts of the location template **/ -CPPCHECKLIB void substituteTemplateLocationStatic(std::string& templateLocation, bool enableColors = false); +CPPCHECKLIB void substituteTemplateLocationStatic(std::string& templateLocation, bool eraseColors = false); /** Get a classification string from the given guideline and reporttype */ CPPCHECKLIB std::string getClassification(const std::string &guideline, ReportType reportType); diff --git a/test/main.cpp b/test/main.cpp index 5ec90ed43df..571b126922d 100644 --- a/test/main.cpp +++ b/test/main.cpp @@ -31,6 +31,7 @@ int main(int argc, char *argv[]) #endif Preprocessor::macroChar = '$'; // While macroChar is char(1) per default outside test suite, we require it to be a human-readable character here. + gDisableColors = true; options args(argc, argv); diff --git a/test/testcolor.cpp b/test/testcolor.cpp index c8086c89fea..7351cb2df9e 100644 --- a/test/testcolor.cpp +++ b/test/testcolor.cpp @@ -30,7 +30,8 @@ class TestColor : public TestFixture { } void toString() const { - ASSERT_EQUALS("\033[31m", ::toString(Color::FgRed)); + // TODO: color conversion is dependent on stdout/stderr being a TTY + ASSERT_EQUALS("", ::toString(Color::FgRed)); } };