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

@@ -1608,15 +1608,9 @@ void CppCheck::reportErr(const ErrorMessage &msg)
     // TODO: only convert if necessary
     const Suppressions::ErrorMessage errorMessage = msg.toSuppressionsErrorMessage();
 
-    if (mUseGlobalSuppressions) {
+    if (mSettings.nomsg.isSuppressed(errorMessage, mUseGlobalSuppressions)) {
-        if (mSettings.nomsg.isSuppressed(errorMessage)) {
         return;
     }
-    } else {
-        if (mSettings.nomsg.isSuppressedLocal(errorMessage)) {
-            return;
-        }
-    }
 
     if (!mSettings.nofail.isSuppressed(errorMessage) && !mSettings.nomsg.isSuppressed(errorMessage)) {
         mExitCode = 1;

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.addSuppressions(std::move(suppressions));
-        settings->nomsg.addSuppression(supp);
     settings->checkHeaders = temp.checkHeaders;
     settings->checkUnusedTemplates = temp.checkUnusedTemplates;
     settings->maxCtuDepth = temp.maxCtuDepth;

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));
         }
     }
 }

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<Suppression> &suppressions)
+std::string Suppressions::addSuppressions(std::list<Suppression> suppressions)
 {
-    for (const auto &newSuppression : suppressions) {
+    for (auto &newSuppression : suppressions) {
-        auto errmsg = addSuppression(newSuppression);
+        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 << "  <suppressions>" << std::endl;

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<Suppression> &suppressions);
+    std::string addSuppressions(std::list<Suppression> 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

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)