Fix #12610 (Justifications for warnings using comments in the code) (#…

…6482) cherry picked from main branch to 2.14.x branch bd694db
danmar · Jun 3, 2024 · 466909f · 466909f
1 parent 2cf9d7b
commit 466909f
Show file tree

Hide file tree

Showing 17 changed files with 314 additions and 114 deletions.
diff --git a/Makefile b/Makefile
diff --git a/gui/erroritem.cpp b/gui/erroritem.cpp
@@ -51,12 +51,10 @@ ErrorItem::ErrorItem(const ErrorMessage &errmsg)
     , cwe(errmsg.cwe.id)
     , hash(errmsg.hash)
     , symbolNames(QString::fromStdString(errmsg.symbolNames()))
+    , remark(QString::fromStdString(errmsg.remark))
 {
-    for (std::list<ErrorMessage::FileLocation>::const_iterator loc = errmsg.callStack.cbegin();
-         loc != errmsg.callStack.cend();
-         ++loc) {
-        errorPath << QErrorPathItem(*loc);
-    }
+    for (const auto& loc: errmsg.callStack)
+        errorPath << QErrorPathItem(loc);
 }
 
 QString ErrorItem::tool() const

diff --git a/gui/erroritem.h b/gui/erroritem.h
@@ -91,6 +91,7 @@ class ErrorItem {
     unsigned long long hash;
     QList<QErrorPathItem> errorPath;
     QString symbolNames;
+    QString remark;
 
     // Special GUI properties
     QString sinceDate;
@@ -122,6 +123,7 @@ class ErrorLine {
     QString message;
     QString sinceDate;
     QString tags;
+    QString remark;
 };
 
 /// @}

diff --git a/gui/resultstree.cpp b/gui/resultstree.cpp
@@ -78,6 +78,7 @@ static constexpr char SINCEDATE[] = "sinceDate";
 static constexpr char SYMBOLNAMES[] = "symbolNames";
 static constexpr char SUMMARY[] = "summary";
 static constexpr char TAGS[] = "tags";
+static constexpr char REMARK[] = "remark";
 
 // These must match column headers given in ResultsTree::translate()
 static constexpr int COLUMN_SINCE_DATE = 6;
@@ -186,6 +187,7 @@ bool ResultsTree::addErrorItem(const ErrorItem &item)
     if (const ProjectFile *activeProject = ProjectFile::getActiveProject()) {
         line.tags = activeProject->getWarningTags(item.hash);
     }
+    line.remark = item.remark;
     //Create the base item for the error and ensure it has a proper
     //file item as a parent
     QStandardItem* fileItem = ensureFileItem(loc.file, item.file0, hide);
@@ -214,6 +216,7 @@ bool ResultsTree::addErrorItem(const ErrorItem &item)
     data[SINCEDATE] = item.sinceDate;
     data[SYMBOLNAMES] = item.symbolNames;
     data[TAGS] = line.tags;
+    data[REMARK] = line.remark;
     data[HIDE] = hide;
     stditem->setData(QVariant(data));
 
@@ -1296,6 +1299,7 @@ void ResultsTree::readErrorItem(const QStandardItem *error, ErrorItem *item) con
     item->file0 = data[FILE0].toString();
     item->sinceDate = data[SINCEDATE].toString();
     item->tags = data[TAGS].toString();
+    item->remark = data[REMARK].toString();
 
     if (error->rowCount() == 0) {
         QErrorPathItem e;

diff --git a/gui/xmlreportv2.cpp b/gui/xmlreportv2.cpp
@@ -49,6 +49,7 @@ static const QString TagsAttribute = "tag";
 static const QString FilenameAttribute = "file";
 static const QString IncludedFromFilenameAttribute = "file0";
 static const QString InconclusiveAttribute = "inconclusive";
+static const QString RemarkAttribute = "remark";
 static const QString InfoAttribute = "info";
 static const QString LineAttribute = "line";
 static const QString ColumnAttribute = "column";
@@ -137,6 +138,8 @@ void XmlReportV2::writeError(const ErrorItem &error)
     mXmlWriter->writeAttribute(VerboseAttribute, message);
     if (error.inconclusive)
         mXmlWriter->writeAttribute(InconclusiveAttribute, "true");
+    if (!error.remark.isEmpty())
+        mXmlWriter->writeAttribute(RemarkAttribute, error.remark);
     if (error.cwe > 0)
         mXmlWriter->writeAttribute(CWEAttribute, QString::number(error.cwe));
     if (error.hash > 0)
@@ -231,6 +234,8 @@ ErrorItem XmlReportV2::readError(const QXmlStreamReader *reader)
         item.message = XmlReport::unquoteMessage(message);
         if (attribs.hasAttribute(QString(), InconclusiveAttribute))
             item.inconclusive = true;
+        if (attribs.hasAttribute(QString(), RemarkAttribute))
+            item.remark = attribs.value(QString(), RemarkAttribute).toString();
         if (attribs.hasAttribute(QString(), CWEAttribute))
             item.cwe = attribs.value(QString(), CWEAttribute).toInt();
         if (attribs.hasAttribute(QString(), HashAttribute))

diff --git a/lib/cppcheck.cpp b/lib/cppcheck.cpp
@@ -706,6 +706,7 @@ unsigned int CppCheck::checkFile(const std::string& filename, const std::string
         }
 
         // Parse comments and then remove them
+        mRemarkComments = preprocessor.getRemarkComments(tokens1);
         preprocessor.inlineSuppressions(tokens1, mSettings.supprs.nomsg);
         if (mSettings.dump || !mSettings.addons.empty()) {
             std::ostringstream oss;
@@ -1613,7 +1614,26 @@ void CppCheck::reportErr(const ErrorMessage &msg)
         mExitCode = 1;
     }
 
-    mErrorLogger.reportErr(msg);
+    std::string remark;
+    if (!msg.callStack.empty()) {
+        for (const auto& r: mRemarkComments) {
+            if (r.file != msg.callStack.back().getfile(false))
+                continue;
+            if (r.lineNumber != msg.callStack.back().line)
+                continue;
+            remark = r.str;
+            break;
+        }
+    }
+
+    if (!remark.empty()) {
+        ErrorMessage msg2(msg);
+        msg2.remark = remark;
+        mErrorLogger.reportErr(msg2);
+    } else {
+        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.supprs.nomsg.isSuppressed(errorMessage)) {
         // add error to plist output file

diff --git a/lib/cppcheck.h b/lib/cppcheck.h
@@ -44,6 +44,7 @@ enum class SHOWTIME_MODES;
 struct FileSettings;
 class CheckUnusedFunctions;
 class Tokenizer;
+class RemarkComment;
 
 namespace simplecpp { class TokenList; }
 
@@ -253,6 +254,8 @@ class CPPCHECKLIB CppCheck : ErrorLogger {
     std::ofstream mPlistFile;
 
     std::unique_ptr<CheckUnusedFunctions> mUnusedFunctionsCheck;
+
+    std::vector<RemarkComment> mRemarkComments;
 };
 
 /// @}

diff --git a/lib/errorlogger.cpp b/lib/errorlogger.cpp
@@ -274,11 +274,9 @@ std::string ErrorMessage::serialize() const
     serializeString(oss, severityToString(severity));
     serializeString(oss, std::to_string(cwe.id));
     serializeString(oss, std::to_string(hash));
+    serializeString(oss, fixInvalidChars(remark));
     serializeString(oss, file0);
-    if (certainty == Certainty::inconclusive) {
-        const std::string text("inconclusive");
-        serializeString(oss, text);
-    }
+    serializeString(oss, (certainty == Certainty::inconclusive) ? "1" : "0");
 
     const std::string saneShortMessage = fixInvalidChars(mShortMessage);
     const std::string saneVerboseMessage = fixInvalidChars(mVerboseMessage);
@@ -312,9 +310,9 @@ void ErrorMessage::deserialize(const std::string &data)
     callStack.clear();
 
     std::istringstream iss(data);
-    std::array<std::string, 7> results;
+    std::array<std::string, 9> results;
     std::size_t elem = 0;
-    while (iss.good() && elem < 7) {
+    while (iss.good() && elem < 9) {
         unsigned int len = 0;
         if (!(iss >> len))
             throw InternalError(nullptr, "Internal Error: Deserialization of error message failed - invalid length");
@@ -332,11 +330,6 @@ void ErrorMessage::deserialize(const std::string &data)
 
             if (!iss.good())
                 throw InternalError(nullptr, "Internal Error: Deserialization of error message failed - premature end of data");
-
-            if (temp == "inconclusive") {
-                certainty = Certainty::inconclusive;
-                continue;
-            }
         }
 
         results[elem++] = std::move(temp);
@@ -345,7 +338,7 @@ void ErrorMessage::deserialize(const std::string &data)
     if (!iss.good())
         throw InternalError(nullptr, "Internal Error: Deserialization of error message failed - premature end of data");
 
-    if (elem != 7)
+    if (elem != 9)
         throw InternalError(nullptr, "Internal Error: Deserialization of error message failed - insufficient elements");
 
     id = std::move(results[0]);
@@ -362,9 +355,12 @@ void ErrorMessage::deserialize(const std::string &data)
         if (!strToInt(results[3], hash, &err))
             throw InternalError(nullptr, "Internal Error: Deserialization of error message failed - invalid hash - " + err);
     }
-    file0 = std::move(results[4]);
-    mShortMessage = std::move(results[5]);
-    mVerboseMessage = std::move(results[6]);
+    remark = std::move(results[4]);
+    file0 = std::move(results[5]);
+    if (results[6] == "1")
+        certainty = Certainty::inconclusive;
+    mShortMessage = std::move(results[7]);
+    mVerboseMessage = std::move(results[8]);
 
     unsigned int stackSize = 0;
     if (!(iss >> stackSize))
@@ -496,6 +492,9 @@ std::string ErrorMessage::toXML() const
     if (!file0.empty())
         printer.PushAttribute("file0", file0.c_str());
 
+    if (!remark.empty())
+        printer.PushAttribute("remark", fixInvalidChars(remark).c_str());
+
     for (std::list<FileLocation>::const_reverse_iterator it = callStack.crbegin(); it != callStack.crend(); ++it) {
         printer.OpenElement("location", false);
         printer.PushAttribute("file", it->getfile().c_str());
@@ -657,6 +656,7 @@ std::string ErrorMessage::toString(bool verbose, const std::string &templateForm
     findAndReplace(result, "{severity}", severityToString(severity));
     findAndReplace(result, "{cwe}", std::to_string(cwe.id));
     findAndReplace(result, "{message}", verbose ? mVerboseMessage : mShortMessage);
+    findAndReplace(result, "{remark}", remark);
     if (!callStack.empty()) {
         if (result.find("{callstack}") != std::string::npos)
             findAndReplace(result, "{callstack}", ErrorLogger::callStackToString(callStack));

diff --git a/lib/errorlogger.h b/lib/errorlogger.h
@@ -171,6 +171,9 @@ class CPPCHECKLIB ErrorMessage {
     CWE cwe;
     Certainty certainty;
 
+    /** remark from REMARK comment */
+    std::string remark;
+
     /** Warning hash */
     std::size_t hash;
 

diff --git a/lib/preprocessor.cpp b/lib/preprocessor.cpp
@@ -167,6 +167,19 @@ static bool parseInlineSuppressionCommentToken(const simplecpp::Token *tok, std:
     return true;
 }
 
+static std::string getRelativeFilename(const simplecpp::Token* tok, const Settings &settings) {
+    std::string relativeFilename(tok->location.file());
+    if (settings.relativePaths) {
+        for (const std::string & basePath : settings.basePaths) {
+            const std::string bp = basePath + "/";
+            if (relativeFilename.compare(0,bp.size(),bp)==0) {
+                relativeFilename = relativeFilename.substr(bp.size());
+            }
+        }
+    }
+    return Path::simplifyPath(relativeFilename);
+}
+
 static void addInlineSuppressions(const simplecpp::TokenList &tokens, const Settings &settings, SuppressionList &suppressions, std::list<BadInlineSuppression> &bad)
 {
     std::list<SuppressionList::Suppression> inlineSuppressionsBlockBegin;
@@ -207,16 +220,7 @@ static void addInlineSuppressions(const simplecpp::TokenList &tokens, const Sett
             continue;
 
         // Relative filename
-        std::string relativeFilename(tok->location.file());
-        if (settings.relativePaths) {
-            for (const std::string & basePath : settings.basePaths) {
-                const std::string bp = basePath + "/";
-                if (relativeFilename.compare(0,bp.size(),bp)==0) {
-                    relativeFilename = relativeFilename.substr(bp.size());
-                }
-            }
-        }
-        relativeFilename = Path::simplifyPath(relativeFilename);
+        const std::string relativeFilename = getRelativeFilename(tok, settings);
 
         // Macro name
         std::string macroName;
@@ -309,6 +313,17 @@ void Preprocessor::inlineSuppressions(const simplecpp::TokenList &tokens, Suppre
     }
 }
 
+std::vector<RemarkComment> Preprocessor::getRemarkComments(const simplecpp::TokenList &tokens) const
+{
+    std::vector<RemarkComment> ret;
+    addRemarkComments(tokens, ret);
+    for (std::map<std::string,simplecpp::TokenList*>::const_iterator it = mTokenLists.cbegin(); it != mTokenLists.cend(); ++it) {
+        if (it->second)
+            addRemarkComments(*it->second, ret);
+    }
+    return ret;
+}
+
 std::list<Directive> Preprocessor::createDirectives(const simplecpp::TokenList &tokens) const
 {
     // directive list..
@@ -1012,3 +1027,52 @@ void Preprocessor::simplifyPragmaAsmPrivate(simplecpp::TokenList *tokenList)
             tokenList->deleteToken(tok4->next);
     }
 }
+
+
+void Preprocessor::addRemarkComments(const simplecpp::TokenList &tokens, std::vector<RemarkComment> &remarkComments) const
+{
+    for (const simplecpp::Token *tok = tokens.cfront(); tok; tok = tok->next) {
+        if (!tok->comment)
+            continue;
+
+        const std::string& comment = tok->str();
+
+        // is it a remark comment?
+        const std::string::size_type pos1 = comment.find_first_not_of("/* \t");
+        if (pos1 == std::string::npos)
+            continue;
+        const std::string::size_type pos2 = comment.find_first_of(": \t", pos1);
+        if (pos2 != pos1 + 6 || comment.compare(pos1, 6, "REMARK") != 0)
+            continue;
+        const std::string::size_type pos3 = comment.find_first_not_of(": \t", pos2);
+        if (pos3 == std::string::npos)
+            continue;
+        if (comment.compare(0,2,"/*") == 0 && pos3 + 2 >= tok->str().size())
+            continue;
+
+        const std::string::size_type pos4 = (comment.compare(0,2,"/*") == 0) ? comment.size()-2 : comment.size();
+        const std::string remarkText = comment.substr(pos3, pos4-pos3);
+
+        // Get remarked token
+        const simplecpp::Token* remarkedToken = nullptr;
+        for (const simplecpp::Token* after = tok->next; after; after = after->next) {
+            if (after->comment)
+                continue;
+            remarkedToken = after;
+            break;
+        }
+        for (const simplecpp::Token* prev = tok->previous; prev; prev = prev->previous) {
+            if (prev->comment)
+                continue;
+            if (sameline(prev, tok))
+                remarkedToken = prev;
+            break;
+        }
+
+        // Relative filename
+        const std::string relativeFilename = getRelativeFilename(remarkedToken, mSettings);
+
+        // Add the suppressions.
+        remarkComments.emplace_back(relativeFilename, remarkedToken->location.line, remarkText);
+    }
+}
diff --git a/lib/preprocessor.h b/lib/preprocessor.h
@@ -60,6 +60,24 @@ struct CPPCHECKLIB Directive {
     Directive(std::string _file, const int _linenr, const std::string &_str);
 };
 
+class CPPCHECKLIB RemarkComment {
+public:
+    RemarkComment(std::string file, unsigned int lineNumber, std::string str)
+        : file(std::move(file))
+        , lineNumber(lineNumber)
+        , str(std::move(str))
+    {}
+
+    /** name of file */
+    std::string file;
+
+    /** line number for the code that the remark comment is about */
+    unsigned int lineNumber;
+
+    /** remark text */
+    std::string str;
+};
+
 /// @addtogroup Core
 /// @{
 
@@ -96,6 +114,8 @@ class CPPCHECKLIB WARN_UNUSED Preprocessor {
 
     std::set<std::string> getConfigs(const simplecpp::TokenList &tokens) const;
 
+    std::vector<RemarkComment> getRemarkComments(const simplecpp::TokenList &tokens) const;
+
     void handleErrors(const simplecpp::OutputList &outputList, bool throwError);
 
     bool loadFiles(const simplecpp::TokenList &rawtokens, std::vector<std::string> &files);
@@ -138,6 +158,8 @@ class CPPCHECKLIB WARN_UNUSED Preprocessor {
 
     static bool hasErrors(const simplecpp::OutputList &outputList);
 
+    void addRemarkComments(const simplecpp::TokenList &tokens, std::vector<RemarkComment> &remarkComments) const;
+
     const Settings& mSettings;
     ErrorLogger &mErrorLogger;