Fix #15009: ErrorLogger::readCode: cache code so we don't have to re-read files

cli/cppcheckexecutor.cpp

@@ -661,13 +661,12 @@ void StdLogger::reportErr(const ErrorMessage &msg)
     msgCopy.classification = getClassification(msgCopy.guideline, mSettings.reportType);
 
     // TODO: there should be no need for verbose and default messages here
-    // Don't perform redundant reads for these formats, the code is not needed
-    // for deduplication
-    const bool noCode = mSettings.outputFormat == Settings::OutputFormat::xml ||
-                        mSettings.outputFormat == Settings::OutputFormat::sarif;
+    const bool noContext = mSettings.outputFormat == Settings::OutputFormat::xml ||
+                           mSettings.outputFormat == Settings::OutputFormat::sarif;
+    const ErrorMessage::SourceLineCallback callback = noContext ? nullptr : getSourceLineCallback();
     const std::string msgStr =
         msgCopy.toString(mSettings.verbose, mSettings.templateFormat,
-                         mSettings.templateLocation, noCode);
+                         mSettings.templateLocation, callback);
 
     // Alert only about unique errors
     if (!mSettings.emitDuplicates && !mShownErrors.insert(msgStr).second)

cli/executor.cpp

@@ -46,7 +46,7 @@ bool Executor::hasToLog(const ErrorMessage &msg)
     if (!mSuppressions.nomsg.isSuppressed(msg, {}))
     {
         // TODO: there should be no need for verbose and default messages here
-        std::string errmsg = msg.toString(mSettings.verbose, mSettings.templateFormat, mSettings.templateLocation);
+        std::string errmsg = msg.toString(mSettings.verbose, mSettings.templateFormat, mSettings.templateLocation, nullptr);
         if (errmsg.empty())
             return false;
 

lib/cppcheck.cpp

@@ -210,9 +210,10 @@ class CppCheck::CppCheckLogger : public ErrorLogger
         }
 
         // TODO: there should be no need for the verbose and default messages here
-        // Code is not needed for deduplication
-        const bool noCode = true;
-        std::string errmsg = msg.toString(mSettings.verbose, mSettings.templateFormat, mSettings.templateLocation, noCode);
+        std::string errmsg = msg.toString(mSettings.verbose,
+                                          mSettings.templateFormat,
+                                          mSettings.templateLocation,
+                                          nullptr);
         if (errmsg.empty())
             return;
 

lib/errorlogger.cpp

@@ -638,23 +638,6 @@ std::string ErrorMessage::toXML() const
     return printer.CStr();
 }
 
-// TODO: read info from some shared resource instead?
-static std::string readCode(const std::string &file, int linenr, int column, const char endl[])
-{
-    std::ifstream fin(file);
-    std::string line;
-    while (linenr > 0 && std::getline(fin,line)) {
-        linenr--;
-    }
-    const std::string::size_type endPos = line.find_last_not_of("\r\n\t ");
-    if (endPos + 1 < line.size())
-        line.erase(endPos + 1);
-    std::string::size_type pos = 0;
-    while ((pos = line.find('\t', pos)) != std::string::npos)
-        line[pos] = ' ';
-    return line + endl + std::string((column>0 ? column-1 : 0), ' ') + '^';
-}
-
 static void replaceSpecialChars(std::string& source)
 {
     // Support a few special characters to allow to specific formatting, see http://sourceforge.net/apps/phpbb/cppcheck/viewtopic.php?f=4&t=494&sid=21715d362c0dbafd3791da4d9522f814
@@ -729,7 +712,40 @@ static void replaceColors(std::string& source, bool erase) {
         replace(source, substitutionMapErase);
 }
 
-std::string ErrorMessage::toString(bool verbose, const std::string &templateFormat, const std::string &templateLocation, bool noCode) const
+static std::string formatLine(std::string line, int column, const char endl[])
+{
+    const std::string::size_type endPos = line.find_last_not_of("\r\n\t ");
+    if (endPos + 1 < line.size())
+        line.erase(endPos + 1);
+
+    std::string::size_type pos = 0;
+    while ((pos = line.find('\t', pos)) != std::string::npos)
+        line[pos] = ' ';
+
+    return line + endl + std::string((column>0 ? column-1 : 0), ' ') + '^';
+}
+
+std::string ErrorMessage::directSourceLineCallback(const std::string &file,
+                                                   int linenr,
+                                                   int column,
+                                                   const char endl[],
+                                                   int cachePrio)
+{
+    (void) cachePrio;
+
+    std::ifstream fin(file);
+    std::string line;
+
+    while (linenr > 0 && std::getline(fin, line))
+        --linenr;
+
+    return formatLine(line, column, endl);
+}
+
+std::string ErrorMessage::toString(bool verbose,
+                                   const std::string &templateFormat,
+                                   const std::string &templateLocation,
+                                   const SourceLineCallback &sourceLineCallback) const
 {
     assert(!templateFormat.empty());
 
@@ -770,7 +786,12 @@ std::string ErrorMessage::toString(bool verbose, const std::string &templateForm
                 endl = "\r\n";
             else
                 endl = "\r";
-            const std::string code = noCode ? "" : readCode(callStack.back().getOrigFile(), callStack.back().line, callStack.back().column, endl);
+            const std::string code = sourceLineCallback == nullptr ?
+                                     "" : sourceLineCallback(callStack.back().getOrigFile(),
+                                                             callStack.back().line,
+                                                             callStack.back().column,
+                                                             endl,
+                                                             0);
             findAndReplace(result, "{code}", code);
         }
     } else {
@@ -786,6 +807,7 @@ std::string ErrorMessage::toString(bool verbose, const std::string &templateForm
     }
 
     if (!templateLocation.empty() && callStack.size() >= 2U) {
+        int cachePrio = 1 - static_cast<int>(callStack.size());
         for (const FileLocation &fileLocation : callStack) {
             std::string text = templateLocation;
 
@@ -802,7 +824,12 @@ std::string ErrorMessage::toString(bool verbose, const std::string &templateForm
                     endl = "\r\n";
                 else
                     endl = "\r";
-                const std::string code = noCode ? "" : readCode(fileLocation.getOrigFile(), fileLocation.line, fileLocation.column, endl);
+                const std::string code = sourceLineCallback == nullptr ?
+                                         "" : sourceLineCallback(fileLocation.getOrigFile(),
+                                                                 fileLocation.line,
+                                                                 fileLocation.column,
+                                                                 endl,
+                                                                 cachePrio++);
                 findAndReplace(text, "{code}", code);
             }
             result += '\n' + text;
@@ -1261,3 +1288,89 @@ std::map<std::string, std::string> createGuidelineMapping(ReportType reportType)
 
     return guidelineMapping;
 }
+
+ErrorLogger::SourceCacheEntry::SourceCacheEntry(const std::string &file, int prio)
+    : prio(prio)
+    , file(file)
+    , mStream(std::ifstream(file))
+    , mLinenr(0)
+{}
+
+bool ErrorLogger::SourceCacheEntry::operator<(const ErrorLogger::SourceCacheEntry &rhs) const
+{
+    return (prio > rhs.prio) ||
+           (prio == rhs.prio && file < rhs.file);
+}
+
+std::string ErrorLogger::SourceCacheEntry::getLine(int linenr)
+{
+    if (linenr < mLinenr) {
+        mLinenr = 0;
+        mStream.clear();
+        mStream.seekg(0);
+    }
+
+    while (mLinenr < linenr && std::getline(mStream, mLine))
+        mLinenr++;
+
+    return mLine;
+}
+
+std::string ErrorLogger::sourceLineCallback(const std::string &file,
+                                            int linenr,
+                                            int column,
+                                            const char endl[],
+                                            int cachePrio)
+{
+    const auto heapCompare = [](const std::shared_ptr<SourceCacheEntry> &lhs, const std::shared_ptr<SourceCacheEntry> &rhs) {
+        return *lhs < *rhs;
+    };
+
+    // Decrease priority for all cache entries
+    for (auto &entry : mSourceCache)
+        --entry->prio;
+
+    std::shared_ptr<SourceCacheEntry> entry = nullptr;
+
+    const auto existing = std::find_if(
+        mSourceCache.begin(),
+        mSourceCache.end(),
+        [&] (const std::shared_ptr<SourceCacheEntry> &e) {
+        return e->file == file;
+    }
+        );
+
+    if (existing == mSourceCache.end()) {
+        if (mSourceCache.size() == getSourceCacheSize()) {
+            // Evict the cache entry with lowest priority
+            std::pop_heap(mSourceCache.begin(), mSourceCache.end(), heapCompare);
+            mSourceCache.pop_back();
+        }
+
+        // Insert new entry
+        entry = std::make_shared<SourceCacheEntry>(file, cachePrio);
+        mSourceCache.push_back(entry);
+        std::push_heap(mSourceCache.begin(), mSourceCache.end(), heapCompare);
+    } else {
+        entry = *existing;
+        if (entry->prio < cachePrio) {
+            // Update priority and sort cache
+            entry->prio = cachePrio;
+            std::make_heap(mSourceCache.begin(), mSourceCache.end(), heapCompare);
+        }
+    }
+
+    return formatLine(entry->getLine(linenr), column, endl);
+}
+
+ErrorMessage::SourceLineCallback ErrorLogger::getSourceLineCallback()
+{
+    return [this](const std::string &file,
+                  int linenr,
+                  int column,
+                  const char endl[],
+                  int cachePrio)
+    {
+        return sourceLineCallback(file, linenr, column, endl, cachePrio);
+    };
+}

lib/errorlogger.h

@@ -26,13 +26,16 @@
 
 #include <cstdint>
 #include <ctime>
+#include <fstream>
 #include <list>
 #include <set>
 #include <sstream>
 #include <string>
 #include <utility>
 #include <vector>
 #include <map>
+#include <functional>
+#include <memory>
 
 class Token;
 class TokenList;
@@ -102,6 +105,19 @@ class CPPCHECKLIB ErrorMessage {
         std::string mInfo;
     };
 
+    using SourceLineCallback = std::function<std::string (
+                                                 const std::string &file,
+                                                 int linenr,
+                                                 int column,
+                                                 const char endl[],
+                                                 int cachePrio)>;
+
+    static std::string directSourceLineCallback(const std::string &file,
+                                                int linenr,
+                                                int column,
+                                                const char endl[],
+                                                int cachePrio);
+
     ErrorMessage(std::list<FileLocation> callStack,
                  std::string file1,
                  Severity severity,
@@ -152,13 +168,13 @@ class CPPCHECKLIB ErrorMessage {
      * or template to be used. E.g. "{file}:{line},{severity},{id},{message}"
      * @param templateLocation Format Empty string to use default output format
      * or template to be used. E.g. "{file}:{line},{info}"
-     * @param noCode Always replace {code} with an empty string
+     * @param sourceLineCallback Function used for fetching a line of source code for the error context
      * @return formatted string
      */
     std::string toString(bool verbose,
                          const std::string &templateFormat,
                          const std::string &templateLocation,
-                         bool noCode = false) const;
+                         const SourceLineCallback &sourceLineCallback = directSourceLineCallback) const;
 
     std::string serialize() const;
     /**
@@ -302,8 +318,36 @@ class CPPCHECKLIB ErrorLogger {
         return mCriticalErrorIds.count(id) != 0;
     }
 
+    ErrorMessage::SourceLineCallback getSourceLineCallback();
+
 private:
     static const std::set<std::string> mCriticalErrorIds;
+
+protected:
+    virtual std::size_t getSourceCacheSize() const {
+        return 4;
+    }
+
+    class CPPCHECKLIB SourceCacheEntry {
+    public:
+        explicit SourceCacheEntry(const std::string &file, int prio);
+
+        int prio;
+        std::string file;
+
+        bool operator<(const SourceCacheEntry &rhs) const;
+
+        std::string getLine(int linenr);
+
+    private:
+        std::ifstream mStream;
+        std::string mLine;
+        int mLinenr{0};
+    };
+
+    std::vector<std::shared_ptr<SourceCacheEntry>> mSourceCache;
+
+    std::string sourceLineCallback(const std::string &file, int linenr, int column, const char endl[], int cachePrio);
 };
 
 /// RAII class for reporting progress messages

test/cli/other_test.py

@@ -4864,7 +4864,7 @@ def test_ipc_inline_suppressions(tmp_path):
     assert stderr.splitlines() == []
 
 test_redundant_file_reads_params = [
-    ([],                       3),
+    ([],                       2),
     (['--suppress=zerodiv'],   1),
     (['--template=cppcheck1'], 1),
     (['--xml'],                1),