Merge topic 'cmDepends_tweaks'

87341d8328 cmDepends: Define DependencyMap instead of DependencyVector
5a15c9e7cb cmDepends: Refactor cmDepends::CheckDependencies method
5f6c236481 cmFiletimeCache: Add cmFiletimeCache::Remove method
18c30786a9 cmFileTime: Make cmFileTime::Compare method const

Acked-by: Kitware Robot <kwrobot@kitware.com>
Merge-request: !3156

11 files changed, 116 insertions, 121 deletions

diff --git a/Source/cmDepends.cxx b/Source/cmDepends.cxx
index efadaf1..ed76dbf 100644
--- a/Source/cmDepends.cxx
+++ b/Source/cmDepends.cxx
@@ -2,6 +2,7 @@
    file Copyright.txt or https://cmake.org/licensing for details.  */
 #include "cmDepends.h"
 
+#include "cmFileTime.h"
 #include "cmFileTimeCache.h"
 #include "cmGeneratedFileStream.h"
 #include "cmLocalGenerator.h"
@@ -10,22 +11,15 @@
 
 #include "cmsys/FStream.hxx"
 #include <sstream>
-#include <string.h>
 #include <utility>
 
 cmDepends::cmDepends(cmLocalGenerator* lg, std::string targetDir)
   : LocalGenerator(lg)
   , TargetDirectory(std::move(targetDir))
-  , Dependee(new char[MaxPath])
-  , Depender(new char[MaxPath])
 {
 }
 
-cmDepends::~cmDepends()
-{
-  delete[] this->Dependee;
-  delete[] this->Depender;
-}
+cmDepends::~cmDepends() = default;
 
 bool cmDepends::Write(std::ostream& makeDepends, std::ostream& internalDepends)
 {
@@ -67,7 +61,7 @@ bool cmDepends::Finalize(std::ostream& /*unused*/, std::ostream& /*unused*/)
 
 bool cmDepends::Check(const std::string& makeFile,
                       const std::string& internalFile,
-                      std::map<std::string, DependencyVector>& validDeps)
+                      DependencyMap& validDeps)
 {
   // Check whether dependencies must be regenerated.
   bool okay = true;
@@ -76,9 +70,9 @@ bool cmDepends::Check(const std::string& makeFile,
     // Clear all dependencies so they will be regenerated.
     this->Clear(makeFile);
     cmSystemTools::RemoveFile(internalFile);
+    this->FileTimeCache->Remove(internalFile);
     okay = false;
   }
-
   return okay;
 }
 
@@ -107,48 +101,62 @@ bool cmDepends::WriteDependencies(const std::set<std::string>& /*unused*/,
   return false;
 }
 
-bool cmDepends::CheckDependencies(
-  std::istream& internalDepends, const std::string& internalDependsFileName,
-  std::map<std::string, DependencyVector>& validDeps)
+bool cmDepends::CheckDependencies(std::istream& internalDepends,
+                                  const std::string& internalDependsFileName,
+                                  DependencyMap& validDeps)
 {
+  // Read internal depends file time
+  cmFileTime internalDependsTime;
+  if (!this->FileTimeCache->Load(internalDependsFileName,
+                                 internalDependsTime)) {
+    return false;
+  }
+
   // Parse dependencies from the stream.  If any dependee is missing
   // or newer than the depender then dependencies should be
   // regenerated.
   bool okay = true;
   bool dependerExists = false;
-  DependencyVector* currentDependencies = nullptr;
 
-  while (internalDepends.getline(this->Dependee, this->MaxPath)) {
-    if (this->Dependee[0] == 0 || this->Dependee[0] == '#' ||
-        this->Dependee[0] == '\r') {
+  std::string line;
+  line.reserve(1024);
+  std::string depender;
+  std::string dependee;
+  cmFileTime dependerTime;
+  cmFileTime dependeeTime;
+  std::vector<std::string>* currentDependencies = nullptr;
+
+  while (std::getline(internalDepends, line)) {
+    // Check if this an empty or a comment line
+    if (line.empty() || line.front() == '#') {
       continue;
     }
-    size_t len = internalDepends.gcount() - 1;
-    if (this->Dependee[len - 1] == '\r') {
-      len--;
-      this->Dependee[len] = 0;
+    // Drop carriage return character at the end
+    if (line.back() == '\r') {
+      line.pop_back();
+      if (line.empty()) {
+        continue;
+      }
     }
-    if (this->Dependee[0] != ' ') {
-      memcpy(this->Depender, this->Dependee, len + 1);
-      // Calling FileExists() for the depender here saves in many cases 50%
-      // of the calls to FileExists() further down in the loop. E.g. for
-      // kdelibs/khtml this reduces the number of calls from 184k down to 92k,
-      // or the time for cmake -E cmake_depends from 0.3 s down to 0.21 s.
-      dependerExists = cmSystemTools::FileExists(this->Depender);
+    // Check if this a depender line
+    if (line.front() != ' ') {
+      depender = line;
+      dependerExists = this->FileTimeCache->Load(depender, dependerTime);
       // If we erase validDeps[this->Depender] by overwriting it with an empty
       // vector, we lose dependencies for dependers that have multiple
       // entries. No need to initialize the entry, std::map will do so on first
       // access.
-      currentDependencies = &validDeps[this->Depender];
+      currentDependencies = &validDeps[depender];
       continue;
     }
-    /*
-    // Parse the dependency line.
-    if(!this->ParseDependency(line.c_str()))
-      {
-      continue;
-      }
-      */
+
+    // This is a dependee line
+    dependee = line.substr(1);
+
+    // Add dependee to depender's list
+    if (currentDependencies != nullptr) {
+      currentDependencies->push_back(dependee);
+    }
 
     // Dependencies must be regenerated
     // * if the dependee does not exist
@@ -156,13 +164,8 @@ bool cmDepends::CheckDependencies(
     // * if the depender does not exist, but the dependee is newer than the
     //   depends file
     bool regenerate = false;
-    const std::string dependee(this->Dependee + 1);
-    const std::string depender(this->Depender);
-    if (currentDependencies != nullptr) {
-      currentDependencies->push_back(dependee);
-    }
-
-    if (!cmSystemTools::FileExists(dependee)) {
+    bool dependeeExists = this->FileTimeCache->Load(dependee, dependeeTime);
+    if (!dependeeExists) {
       // The dependee does not exist.
       regenerate = true;
 
@@ -173,44 +176,38 @@ bool cmDepends::CheckDependencies(
             << depender << "\"." << std::endl;
         cmSystemTools::Stdout(msg.str());
       }
-    } else {
-      if (dependerExists) {
-        // The dependee and depender both exist.  Compare file times.
-        int result = 0;
-        if ((!this->FileTimeCache->Compare(depender, dependee, &result) ||
-             result < 0)) {
-          // The depender is older than the dependee.
-          regenerate = true;
-
-          // Print verbose output.
-          if (this->Verbose) {
-            std::ostringstream msg;
-            msg << "Dependee \"" << dependee << "\" is newer than depender \""
-                << depender << "\"." << std::endl;
-            cmSystemTools::Stdout(msg.str());
-          }
+    } else if (dependerExists) {
+      // The dependee and depender both exist.  Compare file times.
+      if (dependerTime.Older(dependeeTime)) {
+        // The depender is older than the dependee.
+        regenerate = true;
+
+        // Print verbose output.
+        if (this->Verbose) {
+          std::ostringstream msg;
+          msg << "Dependee \"" << dependee << "\" is newer than depender \""
+              << depender << "\"." << std::endl;
+          cmSystemTools::Stdout(msg.str());
         }
-      } else {
-        // The dependee exists, but the depender doesn't. Regenerate if the
-        // internalDepends file is older than the dependee.
-        int result = 0;
-        if ((!this->FileTimeCache->Compare(internalDependsFileName, dependee,
-                                           &result) ||
-             result < 0)) {
-          // The depends-file is older than the dependee.
-          regenerate = true;
-
-          // Print verbose output.
-          if (this->Verbose) {
-            std::ostringstream msg;
-            msg << "Dependee \"" << dependee
-                << "\" is newer than depends file \""
-                << internalDependsFileName << "\"." << std::endl;
-            cmSystemTools::Stdout(msg.str());
-          }
+      }
+    } else {
+      // The dependee exists, but the depender doesn't. Regenerate if the
+      // internalDepends file is older than the dependee.
+      if (internalDependsTime.Older(dependeeTime)) {
+        // The depends-file is older than the dependee.
+        regenerate = true;
+
+        // Print verbose output.
+        if (this->Verbose) {
+          std::ostringstream msg;
+          msg << "Dependee \"" << dependee
+              << "\" is newer than depends file \"" << internalDependsFileName
+              << "\"." << std::endl;
+          cmSystemTools::Stdout(msg.str());
         }
       }
     }
+
     if (regenerate) {
       // Dependencies must be regenerated.
       okay = false;
@@ -218,13 +215,14 @@ bool cmDepends::CheckDependencies(
       // Remove the information of this depender from the map, it needs
       // to be rescanned
       if (currentDependencies != nullptr) {
-        validDeps.erase(this->Depender);
+        validDeps.erase(depender);
         currentDependencies = nullptr;
       }
 
       // Remove the depender to be sure it is rebuilt.
       if (dependerExists) {
         cmSystemTools::RemoveFile(depender);
+        this->FileTimeCache->Remove(depender);
         dependerExists = false;
       }
     }
diff --git a/Source/cmDepends.h b/Source/cmDepends.h
index fc6571d..b7475f0 100644
--- a/Source/cmDepends.h
+++ b/Source/cmDepends.h
@@ -8,7 +8,6 @@
 #include <iosfwd>
 #include <map>
 #include <set>
-#include <stddef.h>
 #include <string>
 #include <vector>
 
@@ -25,6 +24,9 @@ class cmLocalGenerator;
 class cmDepends
 {
 public:
+  typedef std::map<std::string, std::vector<std::string>> DependencyMap;
+
+public:
   /** Instances need to know the build directory name and the relative
       path from the build directory to the target file.  */
   cmDepends(cmLocalGenerator* lg = nullptr, std::string targetDir = "");
@@ -56,17 +58,13 @@ public:
   /** Write dependencies for the target file.  */
   bool Write(std::ostream& makeDepends, std::ostream& internalDepends);
 
-  class DependencyVector : public std::vector<std::string>
-  {
-  };
-
   /** Check dependencies for the target file.  Returns true if
       dependencies are okay and false if they must be generated.  If
       they must be generated Clear has already been called to wipe out
       the old dependencies.
       Dependencies which are still valid will be stored in validDeps. */
   bool Check(const std::string& makeFile, const std::string& internalFile,
-             std::map<std::string, DependencyVector>& validDeps);
+             DependencyMap& validDeps);
 
   /** Clear dependencies for the target file so they will be regenerated.  */
   void Clear(const std::string& file);
@@ -85,9 +83,9 @@ protected:
   // Check dependencies for the target file in the given stream.
   // Return false if dependencies must be regenerated and true
   // otherwise.
-  virtual bool CheckDependencies(
-    std::istream& internalDepends, const std::string& internalDependsFileName,
-    std::map<std::string, DependencyVector>& validDeps);
+  virtual bool CheckDependencies(std::istream& internalDepends,
+                                 const std::string& internalDependsFileName,
+                                 DependencyMap& validDeps);
 
   // Finalize the dependency information for the target.
   virtual bool Finalize(std::ostream& makeDepends,
@@ -105,10 +103,6 @@ protected:
   // The full path to the target's build directory.
   std::string TargetDirectory;
 
-  size_t MaxPath = 16384;
-  char* Dependee;
-  char* Depender;
-
   // The include file search path.
   std::vector<std::string> IncludePath;
 
diff --git a/Source/cmDependsC.cxx b/Source/cmDependsC.cxx
index 7b78767..dc49c18 100644
--- a/Source/cmDependsC.cxx
+++ b/Source/cmDependsC.cxx
@@ -21,9 +21,8 @@
 
 cmDependsC::cmDependsC() = default;
 
-cmDependsC::cmDependsC(
-  cmLocalGenerator* lg, const std::string& targetDir, const std::string& lang,
-  const std::map<std::string, DependencyVector>* validDeps)
+cmDependsC::cmDependsC(cmLocalGenerator* lg, const std::string& targetDir,
+                       const std::string& lang, const DependencyMap* validDeps)
   : cmDepends(lg, targetDir)
   , ValidDeps(validDeps)
 {
@@ -102,8 +101,7 @@ bool cmDependsC::WriteDependencies(const std::set<std::string>& sources,
     this->LocalGenerator->MaybeConvertToRelativePath(binDir, obj);
 
   if (this->ValidDeps != nullptr) {
-    std::map<std::string, DependencyVector>::const_iterator tmpIt =
-      this->ValidDeps->find(obj_i);
+    auto const tmpIt = this->ValidDeps->find(obj_i);
     if (tmpIt != this->ValidDeps->end()) {
       dependencies.insert(tmpIt->second.begin(), tmpIt->second.end());
       haveDeps = true;
diff --git a/Source/cmDependsC.h b/Source/cmDependsC.h
index eee5ae1..3fc839e 100644
--- a/Source/cmDependsC.h
+++ b/Source/cmDependsC.h
@@ -27,8 +27,7 @@ public:
       relative path from the build directory to the target file.  */
   cmDependsC();
   cmDependsC(cmLocalGenerator* lg, const std::string& targetDir,
-             const std::string& lang,
-             const std::map<std::string, DependencyVector>* validDeps);
+             const std::string& lang, const DependencyMap* validDeps);
 
   /** Virtual destructor to cleanup subclasses properly.  */
   ~cmDependsC() override;
@@ -81,7 +80,7 @@ public:
   };
 
 protected:
-  const std::map<std::string, DependencyVector>* ValidDeps = nullptr;
+  const DependencyMap* ValidDeps = nullptr;
   std::set<std::string> Encountered;
   std::queue<UnscannedEntry> Unscanned;
 
diff --git a/Source/cmDependsJava.cxx b/Source/cmDependsJava.cxx
index 2485e15..b17b2ba 100644
--- a/Source/cmDependsJava.cxx
+++ b/Source/cmDependsJava.cxx
@@ -24,8 +24,7 @@ bool cmDependsJava::WriteDependencies(const std::set<std::string>& sources,
 
 bool cmDependsJava::CheckDependencies(
   std::istream& /*internalDepends*/,
-  const std::string& /*internalDependsFileName*/,
-  std::map<std::string, DependencyVector>& /*validDeps*/)
+  const std::string& /*internalDependsFileName*/, DependencyMap& /*validDeps*/)
 {
   return true;
 }
diff --git a/Source/cmDependsJava.h b/Source/cmDependsJava.h
index 109ef13..dd671a1 100644
--- a/Source/cmDependsJava.h
+++ b/Source/cmDependsJava.h
@@ -8,7 +8,6 @@
 #include "cmDepends.h"
 
 #include <iosfwd>
-#include <map>
 #include <set>
 #include <string>
 
@@ -33,9 +32,9 @@ protected:
   bool WriteDependencies(const std::set<std::string>& sources,
                          const std::string& file, std::ostream& makeDepends,
                          std::ostream& internalDepends) override;
-  bool CheckDependencies(
-    std::istream& internalDepends, const std::string& internalDependsFileName,
-    std::map<std::string, DependencyVector>& validDeps) override;
+  bool CheckDependencies(std::istream& internalDepends,
+                         const std::string& internalDependsFileName,
+                         DependencyMap& validDeps) override;
 };
 
 #endif
diff --git a/Source/cmFileTime.h b/Source/cmFileTime.h
index 4c8e556..d4de4e0 100644
--- a/Source/cmFileTime.h
+++ b/Source/cmFileTime.h
@@ -50,7 +50,7 @@ public:
    * @brief Compare file modification times.
    * @return -1, 0, +1 for this older, same, or newer than ftm.
    */
-  int Compare(cmFileTime const& ftm)
+  int Compare(cmFileTime const& ftm) const
   {
     NSC const diff = this->NS - ftm.NS;
     if (diff == 0) {
diff --git a/Source/cmFileTimeCache.cxx b/Source/cmFileTimeCache.cxx
index 1fff6a9..24d6bf6 100644
--- a/Source/cmFileTimeCache.cxx
+++ b/Source/cmFileTimeCache.cxx
@@ -14,8 +14,8 @@ bool cmFileTimeCache::Load(std::string const& fileName, cmFileTime& fileTime)
 {
   // Use the stored time if available.
   {
-    auto fit = this->FileTimes.find(fileName);
-    if (fit != this->FileTimes.end()) {
+    auto fit = this->Cache.find(fileName);
+    if (fit != this->Cache.end()) {
       fileTime = fit->second;
       return true;
     }
@@ -25,10 +25,15 @@ bool cmFileTimeCache::Load(std::string const& fileName, cmFileTime& fileTime)
     return false;
   }
   // Store file time in cache
-  this->FileTimes[fileName] = fileTime;
+  this->Cache[fileName] = fileTime;
   return true;
 }
 
+bool cmFileTimeCache::Remove(std::string const& fileName)
+{
+  return (this->Cache.erase(fileName) != 0);
+}
+
 bool cmFileTimeCache::Compare(std::string const& f1, std::string const& f2,
                               int* result)
 {
diff --git a/Source/cmFileTimeCache.h b/Source/cmFileTimeCache.h
index a47904c..4f1a3a2 100644
--- a/Source/cmFileTimeCache.h
+++ b/Source/cmFileTimeCache.h
@@ -5,7 +5,7 @@
 
 #include "cmConfigure.h" // IWYU pragma: keep
 
-#include "cmFileTime.h"
+#include "cmFileTime.h" // IWYU pragma: keep
 #include <string>
 #include <unordered_map>
 
@@ -28,6 +28,12 @@ public:
   bool Load(std::string const& fileName, cmFileTime& fileTime);
 
   /**
+   * @brief Removes a file time from the cache
+   * @return true if the file was found in the cache and removed
+   */
+  bool Remove(std::string const& fileName);
+
+  /**
    * @brief Compare file modification times.
    * @return true for successful comparison and false for error.
    *
@@ -44,8 +50,7 @@ public:
   bool DifferS(std::string const& f1, std::string const& f2);
 
 private:
-  typedef std::unordered_map<std::string, cmFileTime> FileTimeMap;
-  FileTimeMap FileTimes;
+  std::unordered_map<std::string, cmFileTime> Cache;
 };
 
 #endif
diff --git a/Source/cmLocalUnixMakefileGenerator3.cxx b/Source/cmLocalUnixMakefileGenerator3.cxx
index f6ee9fc..bece12e 100644
--- a/Source/cmLocalUnixMakefileGenerator3.cxx
+++ b/Source/cmLocalUnixMakefileGenerator3.cxx
@@ -1313,7 +1313,7 @@ bool cmLocalUnixMakefileGenerator3::UpdateDependencies(
   // The build.make file may have explicit dependencies for the object
   // files but these will not affect the scanning process so they need
   // not be considered.
-  std::map<std::string, cmDepends::DependencyVector> validDependencies;
+  cmDepends::DependencyMap validDependencies;
   bool needRescanDependencies = false;
   if (!needRescanDirInfo) {
     cmDependsC checker;
@@ -1353,8 +1353,7 @@ bool cmLocalUnixMakefileGenerator3::UpdateDependencies(
 
 bool cmLocalUnixMakefileGenerator3::ScanDependencies(
   std::string const& targetDir, std::string const& dependFile,
-  std::string const& internalDependFile,
-  std::map<std::string, cmDepends::DependencyVector>& validDeps)
+  std::string const& internalDependFile, cmDepends::DependencyMap& validDeps)
 {
   // Read the directory information file.
   cmMakefile* mf = this->Makefile;
diff --git a/Source/cmLocalUnixMakefileGenerator3.h b/Source/cmLocalUnixMakefileGenerator3.h
index ba882da..7a0ea98 100644
--- a/Source/cmLocalUnixMakefileGenerator3.h
+++ b/Source/cmLocalUnixMakefileGenerator3.h
@@ -143,8 +143,7 @@ public:
 
   // File pairs for implicit dependency scanning.  The key of the map
   // is the depender and the value is the explicit dependee.
-  struct ImplicitDependFileMap
-    : public std::map<std::string, cmDepends::DependencyVector>
+  struct ImplicitDependFileMap : public cmDepends::DependencyMap
   {
   };
   struct ImplicitDependLanguageMap
@@ -230,10 +229,10 @@ protected:
                           const char* filename = nullptr);
 
   // Helper methods for dependency updates.
-  bool ScanDependencies(
-    std::string const& targetDir, std::string const& dependFile,
-    std::string const& internalDependFile,
-    std::map<std::string, cmDepends::DependencyVector>& validDeps);
+  bool ScanDependencies(std::string const& targetDir,
+                        std::string const& dependFile,
+                        std::string const& internalDependFile,
+                        cmDepends::DependencyMap& validDeps);
   void CheckMultipleOutputs(bool verbose);
 
 private: