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

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;
+    } 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());
-          }
+        // 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;
+      }
+    } 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());
-          }
+        // 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;
       }
     }

Source/cmDepends.h

@@ -8,7 +8,6 @@
 #include <iosfwd>
 #include <map>
 #include <set>
-#include <stddef.h>
 #include <string>
 #include <vector>
 
@@ -24,6 +23,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.  */
@@ -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;
 

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;

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;
 

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

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

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

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

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>
 
@@ -27,6 +27,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

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;

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: