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 ||
+    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)

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 @@
     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,38 @@
         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)
+{
+    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,
+                                   SourceLineCallback sourceLineCallback) const
 {
     assert(!templateFormat.empty());
 
@@ -770,7 +784,12 @@
                 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 {
@@ -785,6 +804,7 @@
         replace(result, callStackSubstitutionMap);
     }
 
+    int cachePrio = -1;
     if (!templateLocation.empty() && callStack.size() >= 2U) {
         for (const FileLocation &fileLocation : callStack) {
             std::string text = templateLocation;
@@ -802,7 +822,12 @@
                     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 +1286,71 @@
 
     return guidelineMapping;
 }
+
+ErrorLogger::SourceCacheEntry::SourceCacheEntry(const std::string &file, int prio)
+    : prio(prio)
+    , file(file)
+    , stream(std::ifstream(file))
+{
+}
+
+std::string ErrorLogger::sourceLineCallback(const std::string &file,
+                                            int linenr,
+                                            int column,
+                                            const char endl[],
+                                            int cachePrio)
+{
+    // For sorting cache entries by priority
+    const auto heapCompare = [](const std::shared_ptr<SourceCacheEntry> &lhs, const std::shared_ptr<SourceCacheEntry> &rhs) {
+        return lhs->prio > rhs->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() == mSourceCacheSize) {
+            // 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);
+        }
+    }
+
+    entry->stream.clear();
+    entry->stream.seekg(0);
+
+    std::string line;
+    while (linenr > 0 && std::getline(entry->stream, line))
+        linenr--;
+
+    return formatLine(line, 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;
+                         SourceLineCallback sourceLineCallback = directSourceLineCallback) const;
 
     std::string serialize() const;
     /**
@@ -302,8 +318,23 @@ class CPPCHECKLIB ErrorLogger {
         return mCriticalErrorIds.count(id) != 0;
     }
 
+    ErrorMessage::SourceLineCallback getSourceLineCallback();
+
 private:
     static const std::set<std::string> mCriticalErrorIds;
+    static const std::size_t mSourceCacheSize = 4;
+
+    struct SourceCacheEntry {
+        explicit SourceCacheEntry(const std::string &file, int prio);
+
+        int           prio;
+        std::string   file;
+        std::ifstream stream;
+    };
+
+    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),

test/testerrorlogger.cpp

@@ -472,7 +472,7 @@ class TestErrorLogger : public TestFixture {
         ASSERT_EQUALS(1, msg.callStack.size());
         const bool noCode = true;
         ASSERT_EQUALS("code.cpp:3:5: error: Programming error. [errorId]\n",
-                      msg.toString(false, "{file}:{line}:{column}: {severity}:{inconclusive:inconclusive:} {message} [{id}]\n{code}", "", noCode));
+                      msg.toString(false, "{file}:{line}:{column}: {severity}:{inconclusive:inconclusive:} {message} [{id}]\n{code}", "", nullptr));
     }
 
     void CustomFormat() const {