From a77ab9759c4e3a8c37ac9416f097c22c104dd14e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20St=C3=B6neberg?= Date: Thu, 4 May 2023 18:15:18 +0200 Subject: [PATCH] Suppressions: some cleanups (#4980) * Suppressions: merged `isSuppressedLocal()` into `isSuppressed()` * avoid some unnecessary copies when adding suppressions * TestSuppressions: improved readability of multiple line string literals * supressions.h: got rid of unnecessary copy and assignment operators for `Suppressions::Suppression` - fixes `performance-move-const-arg` clang-tidy warning * TestSuppressions: cleaned up a variable construction --- lib/cppcheck.cpp | 10 ++------- lib/importproject.cpp | 3 +-- lib/preprocessor.cpp | 2 +- lib/suppressions.cpp | 32 +++++++++------------------ lib/suppressions.h | 29 ++++-------------------- test/testsuppressions.cpp | 46 ++++++++++++++++++++++++++++++--------- 6 files changed, 54 insertions(+), 68 deletions(-) diff --git a/lib/cppcheck.cpp b/lib/cppcheck.cpp index 0b9578606..11c4a5465 100644 --- a/lib/cppcheck.cpp +++ b/lib/cppcheck.cpp @@ -1608,14 +1608,8 @@ void CppCheck::reportErr(const ErrorMessage &msg) // TODO: only convert if necessary const Suppressions::ErrorMessage errorMessage = msg.toSuppressionsErrorMessage(); - if (mUseGlobalSuppressions) { - if (mSettings.nomsg.isSuppressed(errorMessage)) { - return; - } - } else { - if (mSettings.nomsg.isSuppressedLocal(errorMessage)) { - return; - } + if (mSettings.nomsg.isSuppressed(errorMessage, mUseGlobalSuppressions)) { + return; } if (!mSettings.nofail.isSuppressed(errorMessage) && !mSettings.nomsg.isSuppressed(errorMessage)) { diff --git a/lib/importproject.cpp b/lib/importproject.cpp index 9e5475358..28980a678 100644 --- a/lib/importproject.cpp +++ b/lib/importproject.cpp @@ -1274,8 +1274,7 @@ bool ImportProject::importCppcheckGuiProject(std::istream &istr, Settings *setti for (const std::string &p : paths) guiProject.pathNames.push_back(p); - for (const Suppressions::Suppression &supp : suppressions) - settings->nomsg.addSuppression(supp); + settings->nomsg.addSuppressions(std::move(suppressions)); settings->checkHeaders = temp.checkHeaders; settings->checkUnusedTemplates = temp.checkUnusedTemplates; settings->maxCtuDepth = temp.maxCtuDepth; diff --git a/lib/preprocessor.cpp b/lib/preprocessor.cpp index 622095ee8..f18a872a1 100644 --- a/lib/preprocessor.cpp +++ b/lib/preprocessor.cpp @@ -179,7 +179,7 @@ static void addinlineSuppressions(const simplecpp::TokenList &tokens, const Sett suppr.fileName = relativeFilename; suppr.lineNumber = tok->location.line; suppr.thisAndNextLine = thisAndNextLine; - suppressions.addSuppression(suppr); + suppressions.addSuppression(std::move(suppr)); } } } diff --git a/lib/suppressions.cpp b/lib/suppressions.cpp index 37920e029..7403a3ec4 100644 --- a/lib/suppressions.cpp +++ b/lib/suppressions.cpp @@ -110,7 +110,7 @@ std::string Suppressions::parseXmlFile(const char *filename) return "Unknown suppression element \"" + std::string(e2->Name()) + "\", expected id/fileName/lineNumber/symbolName/hash"; } - const std::string err = addSuppression(s); + const std::string err = addSuppression(std::move(s)); if (!err.empty()) return err; } @@ -218,10 +218,10 @@ std::string Suppressions::addSuppressionLine(const std::string &line) suppression.fileName = Path::simplifyPath(suppression.fileName); - return addSuppression(suppression); + return addSuppression(std::move(suppression)); } -std::string Suppressions::addSuppression(const Suppressions::Suppression &suppression) +std::string Suppressions::addSuppression(Suppressions::Suppression suppression) { // Check if suppression is already in list auto foundSuppression = std::find_if(mSuppressions.begin(), mSuppressions.end(), @@ -253,15 +253,15 @@ std::string Suppressions::addSuppression(const Suppressions::Suppression &suppre if (!isValidGlobPattern(suppression.fileName)) return "Failed to add suppression. Invalid glob pattern '" + suppression.fileName + "'."; - mSuppressions.push_back(suppression); + mSuppressions.push_back(std::move(suppression)); return ""; } -std::string Suppressions::addSuppressions(const std::list &suppressions) +std::string Suppressions::addSuppressions(std::list suppressions) { - for (const auto &newSuppression : suppressions) { - auto errmsg = addSuppression(newSuppression); + for (auto &newSuppression : suppressions) { + auto errmsg = addSuppression(std::move(newSuppression)); if (!errmsg.empty()) return errmsg; } @@ -367,10 +367,12 @@ std::string Suppressions::Suppression::getText() const return ret; } -bool Suppressions::isSuppressed(const Suppressions::ErrorMessage &errmsg) +bool Suppressions::isSuppressed(const Suppressions::ErrorMessage &errmsg, bool global) { const bool unmatchedSuppression(errmsg.errorId == "unmatchedSuppression"); for (Suppression &s : mSuppressions) { + if (!global && !s.isLocal()) + continue; if (unmatchedSuppression && s.errorId != errmsg.errorId) continue; if (s.isMatch(errmsg)) @@ -386,20 +388,6 @@ bool Suppressions::isSuppressed(const ::ErrorMessage &errmsg) return isSuppressed(errmsg.toSuppressionsErrorMessage()); } -bool Suppressions::isSuppressedLocal(const Suppressions::ErrorMessage &errmsg) -{ - const bool unmatchedSuppression(errmsg.errorId == "unmatchedSuppression"); - for (Suppression &s : mSuppressions) { - if (!s.isLocal()) - continue; - if (unmatchedSuppression && s.errorId != errmsg.errorId) - continue; - if (s.isMatch(errmsg)) - return true; - } - return false; -} - void Suppressions::dump(std::ostream & out) const { out << " " << std::endl; diff --git a/lib/suppressions.h b/lib/suppressions.h index df0b75b4f..2f0b339ed 100644 --- a/lib/suppressions.h +++ b/lib/suppressions.h @@ -56,23 +56,8 @@ public: struct CPPCHECKLIB Suppression { Suppression() : lineNumber(NO_LINE), hash(0), thisAndNextLine(false), matched(false), checked(false) {} - Suppression(const Suppression &other) { - *this = other; - } Suppression(std::string id, std::string file, int line=NO_LINE) : errorId(std::move(id)), fileName(std::move(file)), lineNumber(line), hash(0), thisAndNextLine(false), matched(false), checked(false) {} - Suppression & operator=(const Suppression &other) { - errorId = other.errorId; - fileName = other.fileName; - lineNumber = other.lineNumber; - symbolName = other.symbolName; - hash = other.hash; - thisAndNextLine = other.thisAndNextLine; - matched = other.matched; - checked = other.checked; - return *this; - } - bool operator<(const Suppression &other) const { if (errorId != other.errorId) return errorId < other.errorId; @@ -163,21 +148,22 @@ public: * @param suppression suppression details * @return error message. empty upon success */ - std::string addSuppression(const Suppression &suppression); + std::string addSuppression(Suppression suppression); /** * @brief Combine list of suppressions into the current suppressions. * @param suppressions list of suppression details * @return error message. empty upon success */ - std::string addSuppressions(const std::list &suppressions); + std::string addSuppressions(std::list suppressions); /** * @brief Returns true if this message should not be shown to the user. * @param errmsg error message + * @param global use global suppressions * @return true if this error is suppressed. */ - bool isSuppressed(const ErrorMessage &errmsg); + bool isSuppressed(const ErrorMessage &errmsg, bool global = true); /** * @brief Returns true if this message should not be shown to the user. @@ -186,13 +172,6 @@ public: */ bool isSuppressed(const ::ErrorMessage &errmsg); - /** - * @brief Returns true if this message should not be shown to the user, only uses local suppressions. - * @param errmsg error message - * @return true if this error is suppressed. - */ - bool isSuppressedLocal(const ErrorMessage &errmsg); - /** * @brief Create an xml dump of suppressions * @param out stream to write XML to diff --git a/test/testsuppressions.cpp b/test/testsuppressions.cpp index 0dd6e0107..22061415b 100644 --- a/test/testsuppressions.cpp +++ b/test/testsuppressions.cpp @@ -82,6 +82,7 @@ private: TEST_CASE(unusedFunction); TEST_CASE(suppressingSyntaxErrorAndExitCode); + TEST_CASE(suppressLocal); } void suppressionsBadId1() const { @@ -112,7 +113,8 @@ private: void suppressionsDosFormat() const { Suppressions suppressions; - std::istringstream s("abc\r\ndef\r\n"); + std::istringstream s("abc\r\n" + "def\r\n"); ASSERT_EQUALS("", suppressions.parseFile(s)); ASSERT_EQUALS(true, suppressions.isSuppressed(errorMessage("abc"))); ASSERT_EQUALS(true, suppressions.isSuppressed(errorMessage("def"))); @@ -120,7 +122,8 @@ private: void suppressionsFileNameWithColon() const { Suppressions suppressions; - std::istringstream s("errorid:c:\\foo.cpp\nerrorid:c:\\bar.cpp:12"); + std::istringstream s("errorid:c:\\foo.cpp\n" + "errorid:c:\\bar.cpp:12"); ASSERT_EQUALS("", suppressions.parseFile(s)); ASSERT_EQUALS(true, suppressions.isSuppressed(errorMessage("errorid", "c:/foo.cpp", 1111))); ASSERT_EQUALS(false, suppressions.isSuppressed(errorMessage("errorid", "c:/bar.cpp", 10))); @@ -138,7 +141,9 @@ private: // Check that globbing works { Suppressions suppressions; - std::istringstream s("errorid:x*.cpp\nerrorid:y?.cpp\nerrorid:test.c*"); + std::istringstream s("errorid:x*.cpp\n" + "errorid:y?.cpp\n" + "errorid:test.c*"); ASSERT_EQUALS("", suppressions.parseFile(s)); ASSERT_EQUALS(true, suppressions.isSuppressed(errorMessage("errorid", "xyz.cpp", 1))); ASSERT_EQUALS(true, suppressions.isSuppressed(errorMessage("errorid", "xyz.cpp.cpp", 1))); @@ -152,7 +157,10 @@ private: // Check that both a filename match and a glob match apply { Suppressions suppressions; - std::istringstream s("errorid:x*.cpp\nerrorid:xyz.cpp:1\nerrorid:a*.cpp:1\nerrorid:abc.cpp:2"); + std::istringstream s("errorid:x*.cpp\n" + "errorid:xyz.cpp:1\n" + "errorid:a*.cpp:1\n" + "errorid:abc.cpp:2"); ASSERT_EQUALS("", suppressions.parseFile(s)); ASSERT_EQUALS(true, suppressions.isSuppressed(errorMessage("errorid", "xyz.cpp", 1))); ASSERT_EQUALS(true, suppressions.isSuppressed(errorMessage("errorid", "xyz.cpp", 2))); @@ -526,12 +534,14 @@ private: } void suppressionsFileComment() const { - std::istringstream file1("# comment\nabc"); + std::istringstream file1("# comment\n" + "abc"); Suppressions suppressions1; suppressions1.parseFile(file1); ASSERT_EQUALS(true, suppressions1.isSuppressed(errorMessage("abc", "test.cpp", 123))); - std::istringstream file2("// comment\nabc"); + std::istringstream file2("// comment\n" + "abc"); Suppressions suppressions2; suppressions2.parseFile(file2); ASSERT_EQUALS(true, suppressions2.isSuppressed(errorMessage("abc", "test.cpp", 123))); @@ -700,9 +710,9 @@ private: void inlinesuppress_unusedFunction() const { // #4210, #4946 - wrong report of "unmatchedSuppression" for "unusedFunction" Suppressions suppressions; - auto suppression = Suppressions::Suppression("unusedFunction", "test.c", 3); + Suppressions::Suppression suppression("unusedFunction", "test.c", 3); suppression.checked = true; // have to do this because fixes for #5704 - suppressions.addSuppression(suppression); + suppressions.addSuppression(std::move(suppression)); ASSERT_EQUALS(true, !suppressions.getUnmatchedLocalSuppressions("test.c", true).empty()); ASSERT_EQUALS(false, !suppressions.getUnmatchedGlobalSuppressions(true).empty()); ASSERT_EQUALS(false, !suppressions.getUnmatchedLocalSuppressions("test.c", false).empty()); @@ -795,9 +805,11 @@ private: ASSERT_EQUALS(false, s.isSuppressed(errorMsg)); errorMsg.symbolNames = "array1\n"; ASSERT_EQUALS(true, s.isSuppressed(errorMsg)); - errorMsg.symbolNames = "x\narray2\n"; + errorMsg.symbolNames = "x\n" + "array2\n"; ASSERT_EQUALS(true, s.isSuppressed(errorMsg)); - errorMsg.symbolNames = "array3\nx\n"; + errorMsg.symbolNames = "array3\n" + "x\n"; ASSERT_EQUALS(true, s.isSuppressed(errorMsg)); } @@ -833,6 +845,20 @@ private: ASSERT_EQUALS(2, checkSuppression(code3, "zerodiv:test.cpp:3")); // suppress 'errordiv' at line 3 of test.cpp } + void suppressLocal() const { + Suppressions suppressions; + std::istringstream s("errorid:test.cpp\n" + "errorid2"); + ASSERT_EQUALS("", suppressions.parseFile(s)); + ASSERT_EQUALS(true, suppressions.isSuppressed(errorMessage("errorid", "test.cpp", 1))); + ASSERT_EQUALS(true, suppressions.isSuppressed(errorMessage("errorid", "test.cpp", 1), false)); + ASSERT_EQUALS(false, suppressions.isSuppressed(errorMessage("errorid", "test2.cpp", 1))); + ASSERT_EQUALS(false, suppressions.isSuppressed(errorMessage("errorid", "test2.cpp", 1), false)); + ASSERT_EQUALS(true, suppressions.isSuppressed(errorMessage("errorid2", "test.cpp", 1))); + ASSERT_EQUALS(false, suppressions.isSuppressed(errorMessage("errorid2", "test.cpp", 1), false)); + ASSERT_EQUALS(true, suppressions.isSuppressed(errorMessage("errorid2", "test2.cpp", 1))); + ASSERT_EQUALS(false, suppressions.isSuppressed(errorMessage("errorid2", "test2.cpp", 1), false)); + } }; REGISTER_TEST(TestSuppressions)