diff --git a/Makefile b/Makefile index d4e8ad837..92608b760 100644 --- a/Makefile +++ b/Makefile @@ -740,7 +740,7 @@ test/testcondition.o: test/testcondition.cpp externals/simplecpp/simplecpp.h lib test/testconstructors.o: test/testconstructors.cpp lib/addoninfo.h lib/check.h lib/checkclass.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h test/fixture.h $(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testconstructors.cpp -test/testcppcheck.o: test/testcppcheck.cpp lib/addoninfo.h lib/analyzerinfo.h lib/check.h lib/color.h lib/config.h lib/cppcheck.h lib/errorlogger.h lib/errortypes.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/utils.h test/fixture.h +test/testcppcheck.o: test/testcppcheck.cpp lib/addoninfo.h lib/analyzerinfo.h lib/check.h lib/color.h lib/config.h lib/cppcheck.h lib/errorlogger.h lib/errortypes.h lib/filesettings.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/tokenize.h lib/tokenlist.h lib/utils.h test/fixture.h test/helpers.h $(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testcppcheck.cpp test/testerrorlogger.o: test/testerrorlogger.cpp externals/tinyxml2/tinyxml2.h lib/addoninfo.h lib/analyzerinfo.h lib/check.h lib/color.h lib/config.h lib/cppcheck.h lib/errorlogger.h lib/errortypes.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/utils.h lib/xml.h test/fixture.h diff --git a/cli/cppcheckexecutor.cpp b/cli/cppcheckexecutor.cpp index 4320c020f..296a2ad8e 100644 --- a/cli/cppcheckexecutor.cpp +++ b/cli/cppcheckexecutor.cpp @@ -140,7 +140,8 @@ private: /** * Used to filter out duplicate error messages. */ - std::set mShownErrors; + // TODO: store hashes instead of the full messages + std::unordered_set mShownErrors; /** * Report progress time @@ -390,6 +391,8 @@ void CppCheckExecutor::StdLogger::reportErr(const ErrorMessage &msg) return; } + // TODO: we generate a different message here then we log below + // TODO: there should be no need for verbose and default messages here // Alert only about unique errors if (!mShownErrors.insert(msg.toString(mSettings.verbose)).second) return; diff --git a/cli/executor.cpp b/cli/executor.cpp index 40ae2437b..5282376a7 100644 --- a/cli/executor.cpp +++ b/cli/executor.cpp @@ -37,15 +37,21 @@ Executor::Executor(const std::list> &files, assert(!(!files.empty() && !fileSettings.empty())); } +// TODO: this logic is duplicated in CppCheck::reportErr() bool Executor::hasToLog(const ErrorMessage &msg) { + if (!mSettings.library.reportErrors(msg.file0)) + return false; + if (!mSuppressions.isSuppressed(msg, {})) { + // TODO: there should be no need for verbose and default messages here std::string errmsg = msg.toString(mSettings.verbose); + if (errmsg.empty()) + return false; std::lock_guard lg(mErrorListSync); - if (std::find(mErrorList.cbegin(), mErrorList.cend(), errmsg) == mErrorList.cend()) { - mErrorList.emplace_back(std::move(errmsg)); + if (mErrorList.emplace(std::move(errmsg)).second) { return true; } } diff --git a/cli/executor.h b/cli/executor.h index da6c3c9fc..a2f377674 100644 --- a/cli/executor.h +++ b/cli/executor.h @@ -23,6 +23,7 @@ #include #include #include +#include #include class Settings; @@ -74,7 +75,8 @@ protected: private: std::mutex mErrorListSync; - std::list mErrorList; + // TODO: store hashes instead of the full messages + std::unordered_set mErrorList; }; /// @} diff --git a/lib/cppcheck.cpp b/lib/cppcheck.cpp index 53705ed10..9d5b0537c 100644 --- a/lib/cppcheck.cpp +++ b/lib/cppcheck.cpp @@ -1565,6 +1565,7 @@ void CppCheck::purgedConfigurationMessage(const std::string &file, const std::st //--------------------------------------------------------------------------- +// TODO: part of this logic is duplicated in Executor::hasToLog() void CppCheck::reportErr(const ErrorMessage &msg) { if (msg.severity == Severity::none && (msg.id == "logChecker" || endsWith(msg.id, "-logChecker"))) { @@ -1575,17 +1576,6 @@ void CppCheck::reportErr(const ErrorMessage &msg) if (!mSettings.library.reportErrors(msg.file0)) return; - const std::string errmsg = msg.toString(mSettings.verbose); - if (errmsg.empty()) - return; - - // Alert only about unique errors - if (std::find(mErrorList.cbegin(), mErrorList.cend(), errmsg) != mErrorList.cend()) - return; - - if (!mSettings.buildDir.empty()) - mAnalyzerInformation.reportErr(msg); - std::set macroNames; if (!msg.callStack.empty()) { const std::string &file = msg.callStack.back().getfile(false); @@ -1602,12 +1592,24 @@ void CppCheck::reportErr(const ErrorMessage &msg) return; } + // TODO: there should be no need for the verbose and default messages here + std::string errmsg = msg.toString(mSettings.verbose); + if (errmsg.empty()) + return; + + // Alert only about unique errors. + // This makes sure the errors of a single check() call are unique. + // TODO: get rid of this? This is forwarded to another ErrorLogger which is also doing this + if (!mErrorList.emplace(std::move(errmsg)).second) + return; + + if (!mSettings.buildDir.empty()) + mAnalyzerInformation.reportErr(msg); + if (!mSettings.nofail.isSuppressed(errorMessage) && !mSettings.nomsg.isSuppressed(errorMessage)) { mExitCode = 1; } - mErrorList.push_back(errmsg); - mErrorLogger.reportErr(msg); // check if plistOutput should be populated and the current output file is open and the error is not suppressed if (!mSettings.plistOutput.empty() && mPlistFile.is_open() && !mSettings.nomsg.isSuppressed(errorMessage)) { diff --git a/lib/cppcheck.h b/lib/cppcheck.h index 4ed6c9188..8f31d5d75 100644 --- a/lib/cppcheck.h +++ b/lib/cppcheck.h @@ -35,6 +35,7 @@ #include #include #include +#include #include #include @@ -215,7 +216,8 @@ private: */ void reportOut(const std::string &outmsg, Color c = Color::Reset) override; - std::list mErrorList; + // TODO: store hashes instead of the full messages + std::unordered_set mErrorList; Settings mSettings; void reportProgress(const std::string &filename, const char stage[], const std::size_t value) override; diff --git a/lib/errorlogger.cpp b/lib/errorlogger.cpp index 09aeba231..fbb635c69 100644 --- a/lib/errorlogger.cpp +++ b/lib/errorlogger.cpp @@ -618,11 +618,14 @@ static void replaceColors(std::string& source) { replace(source, substitutionMap); } +// TODO: remove default parameters std::string ErrorMessage::toString(bool verbose, const std::string &templateFormat, const std::string &templateLocation) const { // Save this ErrorMessage in plain text. + // TODO: should never happen - remove this // No template is given + // (not 100%) equivalent templateFormat: {callstack} ({severity}{inconclusive:, inconclusive}) {message} if (templateFormat.empty()) { std::string text; if (!callStack.empty()) { diff --git a/test/helpers.h b/test/helpers.h index cc4634ea7..ae4c49b1a 100644 --- a/test/helpers.h +++ b/test/helpers.h @@ -70,6 +70,10 @@ public: return mFullPath; } + const std::string& name() const { + return mName; + } + ScopedFile(const ScopedFile&) = delete; ScopedFile(ScopedFile&&) = delete; ScopedFile& operator=(const ScopedFile&) = delete; diff --git a/test/testcppcheck.cpp b/test/testcppcheck.cpp index 60726a425..1932397c4 100644 --- a/test/testcppcheck.cpp +++ b/test/testcppcheck.cpp @@ -19,7 +19,9 @@ #include "color.h" #include "cppcheck.h" #include "errorlogger.h" +#include "filesettings.h" #include "fixture.h" +#include "helpers.h" #include #include @@ -34,30 +36,37 @@ private: class ErrorLogger2 : public ErrorLogger { public: - std::list id; + std::list ids; + std::list errmsgs; + private: void reportOut(const std::string & /*outmsg*/, Color /*c*/ = Color::Reset) override {} void reportErr(const ErrorMessage &msg) override { - id.push_back(msg.id); + ids.push_back(msg.id); + errmsgs.push_back(msg); } }; void run() override { TEST_CASE(getErrorMessages); + TEST_CASE(checkWithFile); + TEST_CASE(checkWithFS); + TEST_CASE(suppress_error_library); + TEST_CASE(unique_errors); } void getErrorMessages() const { ErrorLogger2 errorLogger; CppCheck::getErrorMessages(errorLogger); - ASSERT(!errorLogger.id.empty()); + ASSERT(!errorLogger.ids.empty()); // Check if there are duplicate error ids in errorLogger.id std::string duplicate; - for (std::list::const_iterator it = errorLogger.id.cbegin(); - it != errorLogger.id.cend(); + for (std::list::const_iterator it = errorLogger.ids.cbegin(); + it != errorLogger.ids.cend(); ++it) { - if (std::find(errorLogger.id.cbegin(), it, *it) != it) { + if (std::find(errorLogger.ids.cbegin(), it, *it) != it) { duplicate = "Duplicate ID: " + *it; break; } @@ -67,7 +76,7 @@ private: // Check for error ids from this class. bool foundPurgedConfiguration = false; bool foundTooManyConfigs = false; - for (const std::string & it : errorLogger.id) { + for (const std::string & it : errorLogger.ids) { if (it == "purgedConfiguration") foundPurgedConfiguration = true; else if (it == "toomanyconfigs") @@ -76,6 +85,102 @@ private: ASSERT(foundPurgedConfiguration); ASSERT(foundTooManyConfigs); } + + void checkWithFile() const + { + ScopedFile file("test.cpp", + "int main()\n" + "{\n" + " int i = *((int*)0);\n" + " return 0;\n" + "}"); + + ErrorLogger2 errorLogger; + CppCheck cppcheck(errorLogger, false, {}); + ASSERT_EQUALS(1, cppcheck.check(file.path())); + // TODO: how to properly disable these warnings? + errorLogger.ids.erase(std::remove_if(errorLogger.ids.begin(), errorLogger.ids.end(), [](const std::string& id) { + return id == "logChecker"; + }), errorLogger.ids.end()); + ASSERT_EQUALS(1, errorLogger.ids.size()); + ASSERT_EQUALS("nullPointer", *errorLogger.ids.cbegin()); + } + + void checkWithFS() const + { + ScopedFile file("test.cpp", + "int main()\n" + "{\n" + " int i = *((int*)0);\n" + " return 0;\n" + "}"); + + ErrorLogger2 errorLogger; + CppCheck cppcheck(errorLogger, false, {}); + FileSettings fs; + fs.filename = file.path(); + ASSERT_EQUALS(1, cppcheck.check(fs)); + // TODO: how to properly disable these warnings? + errorLogger.ids.erase(std::remove_if(errorLogger.ids.begin(), errorLogger.ids.end(), [](const std::string& id) { + return id == "logChecker"; + }), errorLogger.ids.end()); + ASSERT_EQUALS(1, errorLogger.ids.size()); + ASSERT_EQUALS("nullPointer", *errorLogger.ids.cbegin()); + } + + void suppress_error_library() const + { + ScopedFile file("test.cpp", + "int main()\n" + "{\n" + " int i = *((int*)0);\n" + " return 0;\n" + "}"); + + ErrorLogger2 errorLogger; + CppCheck cppcheck(errorLogger, false, {}); + const char xmldata[] = R"()"; + const Settings s = settingsBuilder().libraryxml(xmldata, sizeof(xmldata)).build(); + cppcheck.settings() = s; + ASSERT_EQUALS(0, cppcheck.check(file.path())); + // TODO: how to properly disable these warnings? + errorLogger.ids.erase(std::remove_if(errorLogger.ids.begin(), errorLogger.ids.end(), [](const std::string& id) { + return id == "logChecker"; + }), errorLogger.ids.end()); + ASSERT_EQUALS(0, errorLogger.ids.size()); + } + + // TODO: hwo to actually get duplicated findings + void unique_errors() const + { + ScopedFile file("inc.h", + "inline void f()\n" + "{\n" + " (void)*((int*)0);\n" + "}"); + ScopedFile test_file_a("a.cpp", + "#include \"inc.h\""); + ScopedFile test_file_b("b.cpp", + "#include \"inc.h\""); + + ErrorLogger2 errorLogger; + CppCheck cppcheck(errorLogger, false, {}); + ASSERT_EQUALS(1, cppcheck.check(test_file_a.path())); + ASSERT_EQUALS(1, cppcheck.check(test_file_b.path())); + // TODO: how to properly disable these warnings? + errorLogger.errmsgs.erase(std::remove_if(errorLogger.errmsgs.begin(), errorLogger.errmsgs.end(), [](const ErrorMessage& errmsg) { + return errmsg.id == "logChecker"; + }), errorLogger.errmsgs.end()); + // the internal errorlist is cleared after each check() call + ASSERT_EQUALS(2, errorLogger.errmsgs.size()); + auto it = errorLogger.errmsgs.cbegin(); + ASSERT_EQUALS("nullPointer", it->id); + ++it; + ASSERT_EQUALS("nullPointer", it->id); + } + + // TODO: test suppressions + // TODO: test all with FS }; REGISTER_TEST(TestCppcheck) diff --git a/test/testprocessexecutor.cpp b/test/testprocessexecutor.cpp index 59c5577e8..4cb68c4ae 100644 --- a/test/testprocessexecutor.cpp +++ b/test/testprocessexecutor.cpp @@ -149,6 +149,7 @@ private: TEST_CASE(showtime_file); TEST_CASE(showtime_summary); TEST_CASE(showtime_file_total); + TEST_CASE(suppress_error_library); #endif // !WIN32 } @@ -329,6 +330,34 @@ private: TODO_ASSERT(output_s.find("Check time: " + fprefix() + "_2.cpp: ") != std::string::npos); } + void suppress_error_library() { + SUPPRESS; + const Settings settingsOld = settings; + const char xmldata[] = R"()"; + settings = settingsBuilder().libraryxml(xmldata, sizeof(xmldata)).build(); + check(2, 1, 0, + "int main()\n" + "{\n" + " int i = *((int*)0);\n" + " return 0;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + settings = settingsOld; + } + + void unique_errors() { + SUPPRESS; + ScopedFile inc_h(fprefix() + ".h", + "inline void f()\n" + "{\n" + " (void)*((int*)0);\n" + "}"); + check(2, 2, 2, + "#include \"" + inc_h.name() +"\""); + // this is made unique by the executor + ASSERT_EQUALS("[" + inc_h.name() + ":3]: (error) Null pointer dereference: (int*)0\n", errout.str()); + } + // TODO: test whole program analysis }; diff --git a/test/testsingleexecutor.cpp b/test/testsingleexecutor.cpp index 6ee535838..d769e0b05 100644 --- a/test/testsingleexecutor.cpp +++ b/test/testsingleexecutor.cpp @@ -153,6 +153,8 @@ private: TEST_CASE(showtime_file); TEST_CASE(showtime_summary); TEST_CASE(showtime_file_total); + TEST_CASE(suppress_error_library); + TEST_CASE(unique_errors); } void many_files() { @@ -321,7 +323,36 @@ private: ASSERT(output_s.find("Check time: " + fprefix() + "_" + zpad3(2) + ".cpp: ") != std::string::npos); } + void suppress_error_library() { + SUPPRESS; + const Settings settingsOld = settings; + const char xmldata[] = R"()"; + settings = settingsBuilder().libraryxml(xmldata, sizeof(xmldata)).build(); + check(1, 0, + "int main()\n" + "{\n" + " int i = *((int*)0);\n" + " return 0;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + settings = settingsOld; + } + + void unique_errors() { + SUPPRESS; + ScopedFile inc_h(fprefix() + ".h", + "inline void f()\n" + "{\n" + " (void)*((int*)0);\n" + "}"); + check(2, 2, + "#include \"" + inc_h.name() + "\""); + // these are not actually made unique by the implementation. That needs to be done by the given ErrorLogger + ASSERT_EQUALS("[" + inc_h.name() + ":3]: (error) Null pointer dereference: (int*)0\n", errout.str()); + } + // TODO: test whole program analysis + // TODO: test unique errors }; class TestSingleExecutorFiles : public TestSingleExecutorBase { diff --git a/test/testthreadexecutor.cpp b/test/testthreadexecutor.cpp index ea4e3a9e1..229d0dc3d 100644 --- a/test/testthreadexecutor.cpp +++ b/test/testthreadexecutor.cpp @@ -149,6 +149,8 @@ private: TEST_CASE(showtime_file); TEST_CASE(showtime_summary); TEST_CASE(showtime_file_total); + TEST_CASE(suppress_error_library); + TEST_CASE(unique_errors); } void deadlock_with_many_errors() { @@ -326,6 +328,34 @@ private: ASSERT(output_s.find("Check time: " + fprefix() + "_2.cpp: ") != std::string::npos); } + void suppress_error_library() { + SUPPRESS; + const Settings settingsOld = settings; + const char xmldata[] = R"()"; + settings = settingsBuilder().libraryxml(xmldata, sizeof(xmldata)).build(); + check(2, 1, 0, + "int main()\n" + "{\n" + " int i = *((int*)0);\n" + " return 0;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + settings = settingsOld; + } + + void unique_errors() { + SUPPRESS; + ScopedFile inc_h(fprefix() + ".h", + "inline void f()\n" + "{\n" + " (void)*((int*)0);\n" + "}"); + check(2, 2, 2, + "#include \"" + inc_h.name() +"\""); + // this is made unique by the executor + ASSERT_EQUALS("[" + inc_h.name() + ":3]: (error) Null pointer dereference: (int*)0\n", errout.str()); + } + // TODO: test whole program analysis };