Merge topic 'general_cleanup_enhance'

0a21d820d3 Remove c_str() from calls to converttorelativeformake in XCode Generator f93cc4158e Refactor cmCacheManager::LoadCache to use ostringstream 915b71010c Enhance RunCMake test coverage for file(GLOB) fcaa134c6c Refactor HandleGlobCommand cf5d0b49e8 Adjust class description in cmFileTimeComparison.h Acked-by: Kitware Robot <kwrobot@kitware.com> Merge-request: !1810
2026-05-01 11:50:11 -05:00 · 2018-03-05 14:02:07 +00:00
parent dc38970f83 0a21d820d3
commit 49e777aeb1
19 changed files with 141 additions and 107 deletions
@@ -160,14 +160,14 @@ bool cmCacheManager::LoadCache(const std::string& path, bool internal,
    currentcwd += "/CMakeCache.txt";
    oldcwd += "/CMakeCache.txt";
    if (!cmSystemTools::SameFile(oldcwd, currentcwd)) {
-      std::string message =
-        std::string("The current CMakeCache.txt directory ") + currentcwd +
-        std::string(" is different than the directory ") +
-        std::string(this->GetInitializedCacheValue("CMAKE_CACHEFILE_DIR")) +
-        std::string(" where CMakeCache.txt was created. This may result "
-                    "in binaries being created in the wrong place. If you "
-                    "are not sure, reedit the CMakeCache.txt");
-      cmSystemTools::Error(message.c_str());
+      std::ostringstream message;
+      message << "The current CMakeCache.txt directory " << currentcwd
+              << " is different than the directory "
+              << this->GetInitializedCacheValue("CMAKE_CACHEFILE_DIR")
+              << " where CMakeCache.txt was created. This may result "
+                 "in binaries being created in the wrong place. If you "
+                 "are not sure, reedit the CMakeCache.txt";
+      cmSystemTools::Error(message.str().c_str());
    }
  }
  return true;
@@ -757,9 +757,10 @@ bool cmFileCommand::HandleGlobCommand(std::vector<std::string> const& args,
    }
  }

-  std::string output;
+  std::ostringstream outputStream;
  bool first = true;
-  for (; i != args.end(); ++i) {
+  bool warnFollowedSymlinks = false;
+  while (i != args.end()) {
    if (*i == "LIST_DIRECTORIES") {
      ++i;
      if (i != args.end()) {
@@ -777,103 +778,109 @@ bool cmFileCommand::HandleGlobCommand(std::vector<std::string> const& args,
        this->SetError("LIST_DIRECTORIES missing bool value.");
        return false;
      }
-      continue;
-    }
-
-    if (recurse && (*i == "FOLLOW_SYMLINKS")) {
+      ++i;
+      if (i == args.end()) {
+        this->SetError("GLOB requires a glob expression after the bool.");
+        return false;
+      }
+    } else if (*i == "FOLLOW_SYMLINKS") {
+      if (!recurse) {
+        this->SetError("FOLLOW_SYMLINKS is not a valid parameter for GLOB.");
+        return false;
+      }
      explicitFollowSymlinks = true;
      g.RecurseThroughSymlinksOn();
      ++i;
      if (i == args.end()) {
        this->SetError(
-          "GLOB_RECURSE requires a glob expression after FOLLOW_SYMLINKS");
+          "GLOB_RECURSE requires a glob expression after FOLLOW_SYMLINKS.");
        return false;
      }
-    }
-
-    if (*i == "RELATIVE") {
+    } else if (*i == "RELATIVE") {
      ++i; // skip RELATIVE
      if (i == args.end()) {
-        this->SetError("GLOB requires a directory after the RELATIVE tag");
+        this->SetError("GLOB requires a directory after the RELATIVE tag.");
        return false;
      }
      g.SetRelative(i->c_str());
      ++i;
      if (i == args.end()) {
-        this->SetError("GLOB requires a glob expression after the directory");
+        this->SetError("GLOB requires a glob expression after the directory.");
        return false;
      }
-    }
-
-    cmsys::Glob::GlobMessages globMessages;
-    if (!cmsys::SystemTools::FileIsFullPath(*i)) {
-      std::string expr = this->Makefile->GetCurrentSourceDirectory();
-      // Handle script mode
-      if (!expr.empty()) {
-        expr += "/" + *i;
-        g.FindFiles(expr, &globMessages);
-      } else {
-        g.FindFiles(*i, &globMessages);
-      }
    } else {
-      g.FindFiles(*i, &globMessages);
-    }
-
-    if (!globMessages.empty()) {
-      bool shouldExit = false;
-      for (cmsys::Glob::Message const& globMessage : globMessages) {
-        if (globMessage.type == cmsys::Glob::cyclicRecursion) {
-          this->Makefile->IssueMessage(
-            cmake::AUTHOR_WARNING,
-            "Cyclic recursion detected while globbing for '" + *i + "':\n" +
-              globMessage.content);
+      std::string expr = *i;
+      if (!cmsys::SystemTools::FileIsFullPath(*i)) {
+        expr = this->Makefile->GetCurrentSourceDirectory();
+        // Handle script mode
+        if (!expr.empty()) {
+          expr += "/" + *i;
        } else {
-          this->Makefile->IssueMessage(
-            cmake::FATAL_ERROR, "Error has occurred while globbing for '" +
-              *i + "' - " + globMessage.content);
-          shouldExit = true;
+          expr = *i;
        }
      }
-      if (shouldExit) {
-        return false;
-      }
-    }

-    std::vector<std::string>::size_type cc;
-    std::vector<std::string>& files = g.GetFiles();
-    std::sort(files.begin(), files.end());
-    for (cc = 0; cc < files.size(); cc++) {
-      if (!first) {
-        output += ";";
+      cmsys::Glob::GlobMessages globMessages;
+      g.FindFiles(expr, &globMessages);
+
+      if (!globMessages.empty()) {
+        bool shouldExit = false;
+        for (cmsys::Glob::Message const& globMessage : globMessages) {
+          if (globMessage.type == cmsys::Glob::cyclicRecursion) {
+            this->Makefile->IssueMessage(
+              cmake::AUTHOR_WARNING,
+              "Cyclic recursion detected while globbing for '" + *i + "':\n" +
+                globMessage.content);
+          } else {
+            this->Makefile->IssueMessage(
+              cmake::FATAL_ERROR, "Error has occurred while globbing for '" +
+                *i + "' - " + globMessage.content);
+            shouldExit = true;
+          }
+        }
+        if (shouldExit) {
+          return false;
+        }
      }
-      output += files[cc];
-      first = false;
+
+      if (recurse && !explicitFollowSymlinks &&
+          g.GetFollowedSymlinkCount() != 0) {
+        warnFollowedSymlinks = true;
+      }
+
+      std::vector<std::string>& files = g.GetFiles();
+      std::sort(files.begin(), files.end());
+      for (std::string const& file : files) {
+        if (!first) {
+          outputStream << ";";
+        }
+        outputStream << file;
+        first = false;
+      }
+      ++i;
    }
  }

-  if (recurse && !explicitFollowSymlinks) {
-    switch (status) {
-      case cmPolicies::REQUIRED_IF_USED:
-      case cmPolicies::REQUIRED_ALWAYS:
-      case cmPolicies::NEW:
-        // Correct behavior, yay!
-        break;
-      case cmPolicies::OLD:
-      // Probably not really the expected behavior, but the author explicitly
-      // asked for the old behavior... no warning.
-      case cmPolicies::WARN:
-        // Possibly unexpected old behavior *and* we actually traversed
-        // symlinks without being explicitly asked to: warn the author.
-        if (g.GetFollowedSymlinkCount() != 0) {
-          this->Makefile->IssueMessage(
-            cmake::AUTHOR_WARNING,
-            cmPolicies::GetPolicyWarning(cmPolicies::CMP0009));
-        }
-        break;
-    }
+  switch (status) {
+    case cmPolicies::REQUIRED_IF_USED:
+    case cmPolicies::REQUIRED_ALWAYS:
+    case cmPolicies::NEW:
+      // Correct behavior, yay!
+      break;
+    case cmPolicies::OLD:
+    // Probably not really the expected behavior, but the author explicitly
+    // asked for the old behavior... no warning.
+    case cmPolicies::WARN:
+      // Possibly unexpected old behavior *and* we actually traversed
+      // symlinks without being explicitly asked to: warn the author.
+      if (warnFollowedSymlinks) {
+        this->Makefile->IssueMessage(
+          cmake::AUTHOR_WARNING,
+          cmPolicies::GetPolicyWarning(cmPolicies::CMP0009));
+      }
+      break;
  }
-
-  this->Makefile->AddDefinition(variable, output.c_str());
+  this->Makefile->AddDefinition(variable, outputStream.str().c_str());
  return true;
 }

@@ -8,9 +8,9 @@
 class cmFileTimeComparisonInternal;

 /** \class cmFileTimeComparison
- * \brief Helper class for performing globbing searches.
+ * \brief Helper class for comparing file modification times.
 *
- * Finds all files that match a given globbing expression.
+ * Compare file modification times or test if file modification times differ.
 */
 class cmFileTimeComparison
 {
@@ -466,7 +466,7 @@ void cmGlobalXCodeGenerator::AddExtraTargets(
  if (regenerate && (isTopLevel || !generateTopLevelProjectOnly)) {
    this->CreateReRunCMakeFile(root, gens);
    std::string file =
-      this->ConvertToRelativeForMake(this->CurrentReRunCMakeMakefile.c_str());
+      this->ConvertToRelativeForMake(this->CurrentReRunCMakeMakefile);
    cmSystemTools::ReplaceString(file, "\\ ", " ");
    cmTarget* check = mf->AddUtilityCommand(
      CMAKE_CHECK_BUILD_SYSTEM_TARGET, cmMakefile::TargetOrigin::Generator,
@@ -553,7 +553,7 @@ void cmGlobalXCodeGenerator::CreateReRunCMakeFile(

  for (const auto& lfile : lfiles) {
    makefileStream << "TARGETS += $(subst $(space),$(spaceplus),$(wildcard "
-                   << this->ConvertToRelativeForMake(lfile.c_str()) << "))\n";
+                   << this->ConvertToRelativeForMake(lfile) << "))\n";
  }

  std::string checkCache = root->GetBinaryDirectory();
@@ -562,11 +562,11 @@ void cmGlobalXCodeGenerator::CreateReRunCMakeFile(
  checkCache += "cmake.check_cache";

  makefileStream << "\n"
-                 << this->ConvertToRelativeForMake(checkCache.c_str())
+                 << this->ConvertToRelativeForMake(checkCache)
                 << ": $(TARGETS)\n";
  makefileStream << "\t"
                 << this->ConvertToRelativeForMake(
-                      cmSystemTools::GetCMakeCommand().c_str())
+                      cmSystemTools::GetCMakeCommand())
                 << " -H"
                 << this->ConvertToRelativeForMake(root->GetSourceDirectory())
                 << " -B"
@@ -1571,12 +1571,11 @@ void cmGlobalXCodeGenerator::AddCommandsToBuildPhase(
  }

  std::string cdir = this->CurrentLocalGenerator->GetCurrentBinaryDirectory();
-  cdir = this->ConvertToRelativeForMake(cdir.c_str());
+  cdir = this->ConvertToRelativeForMake(cdir);
  std::string makecmd = "make -C ";
  makecmd += cdir;
  makecmd += " -f ";
-  makecmd +=
-    this->ConvertToRelativeForMake((makefile + "$CONFIGURATION").c_str());
+  makecmd += this->ConvertToRelativeForMake((makefile + "$CONFIGURATION"));
  makecmd += " all";
  buildphase->AddAttribute("shellScript", this->CreateString(makecmd));
  buildphase->AddAttribute("showEnvVarsInLog", this->CreateString("0"));
@@ -1611,8 +1610,7 @@ void cmGlobalXCodeGenerator::CreateCustomRulesMakefile(
      const std::vector<std::string>& outputs = ccg.GetOutputs();
      if (!outputs.empty()) {
        for (auto const& output : outputs) {
-          makefileStream << "\\\n\t"
-                         << this->ConvertToRelativeForMake(output.c_str());
+          makefileStream << "\\\n\t" << this->ConvertToRelativeForMake(output);
        }
      } else {
        std::ostringstream str;
@@ -1633,8 +1631,7 @@ void cmGlobalXCodeGenerator::CreateCustomRulesMakefile(
        // There is at least one output, start the rule for it
        const char* sep = "";
        for (auto const& output : outputs) {
-          makefileStream << sep
-                         << this->ConvertToRelativeForMake(output.c_str());
+          makefileStream << sep << this->ConvertToRelativeForMake(output);
          sep = " ";
        }
        makefileStream << ": ";
@@ -1646,8 +1643,7 @@ void cmGlobalXCodeGenerator::CreateCustomRulesMakefile(
        std::string dep;
        if (this->CurrentLocalGenerator->GetRealDependency(d, configName,
                                                           dep)) {
-          makefileStream << "\\\n"
-                         << this->ConvertToRelativeForMake(dep.c_str());
+          makefileStream << "\\\n" << this->ConvertToRelativeForMake(dep);
        }
      }
      makefileStream << "\n";
@@ -1664,12 +1660,12 @@ void cmGlobalXCodeGenerator::CreateCustomRulesMakefile(
        // Build the command line in a single string.
        std::string cmd2 = ccg.GetCommand(c);
        cmSystemTools::ReplaceString(cmd2, "/./", "/");
-        cmd2 = this->ConvertToRelativeForMake(cmd2.c_str());
+        cmd2 = this->ConvertToRelativeForMake(cmd2);
        std::string cmd;
        std::string wd = ccg.GetWorkingDirectory();
        if (!wd.empty()) {
          cmd += "cd ";
-          cmd += this->ConvertToRelativeForMake(wd.c_str());
+          cmd += this->ConvertToRelativeForMake(wd);
          cmd += " && ";
        }
        cmd += cmd2;
@@ -3190,7 +3186,7 @@ void cmGlobalXCodeGenerator::CreateXCodeDependHackTarget(
          gt->GetType() == cmStateEnums::SHARED_LIBRARY ||
          gt->GetType() == cmStateEnums::MODULE_LIBRARY) {
        std::string tfull = gt->GetFullPath(configName);
-        std::string trel = this->ConvertToRelativeForMake(tfull.c_str());
+        std::string trel = this->ConvertToRelativeForMake(tfull);

        // Add this target to the post-build phases of its dependencies.
        std::map<std::string, cmXCodeObject::StringVec>::const_iterator y =
@@ -3218,7 +3214,7 @@ void cmGlobalXCodeGenerator::CreateXCodeDependHackTarget(
          target->GetDependLibraries().find(configName);
        if (x != target->GetDependLibraries().end()) {
          for (auto const& deplib : x->second) {
-            std::string file = this->ConvertToRelativeForMake(deplib.c_str());
+            std::string file = this->ConvertToRelativeForMake(deplib);
            makefileStream << "\\\n\t" << file;
            dummyRules.insert(file);
          }
@@ -3233,7 +3229,7 @@ void cmGlobalXCodeGenerator::CreateXCodeDependHackTarget(
          d += objLibName;
          d += ".a";

-          std::string dependency = this->ConvertToRelativeForMake(d.c_str());
+          std::string dependency = this->ConvertToRelativeForMake(d);
          makefileStream << "\\\n\t" << dependency;
          dummyRules.insert(dependency);
        }
@@ -3241,8 +3237,7 @@ void cmGlobalXCodeGenerator::CreateXCodeDependHackTarget(
        // Write the action to remove the target if it is out of date.
        makefileStream << "\n";
        makefileStream << "\t/bin/rm -f "
-                       << this->ConvertToRelativeForMake(tfull.c_str())
-                       << "\n";
+                       << this->ConvertToRelativeForMake(tfull) << "\n";
        // if building for more than one architecture
        // then remove those executables as well
        if (this->Architectures.size() > 1) {
@@ -3254,8 +3249,7 @@ void cmGlobalXCodeGenerator::CreateXCodeDependHackTarget(
            universalFile += "/";
            universalFile += gt->GetFullName(configName);
            makefileStream << "\t/bin/rm -f "
-                           << this->ConvertToRelativeForMake(
-                                universalFile.c_str())
+                           << this->ConvertToRelativeForMake(universalFile)
                           << "\n";
          }
        }
@@ -0,0 +1 @@
+1
@@ -0,0 +1,4 @@
+^CMake Error at GLOB-error-FOLLOW_SYMLINKS\.cmake:[0-9]+ \(file\):
+  file FOLLOW_SYMLINKS is not a valid parameter for GLOB\.
+Call Stack \(most recent call first\):
+  CMakeLists\.txt:[0-9]+ \(include\)$
@@ -0,0 +1 @@
+file(GLOB CONTENT_LIST FOLLOW_SYMLINKS)
@@ -0,0 +1 @@
+1
@@ -0,0 +1,4 @@
+^CMake Error at GLOB-error-RELATIVE-no-arg\.cmake:[0-9]+ \(file\):
+  file GLOB requires a directory after the RELATIVE tag\.
+Call Stack \(most recent call first\):
+  CMakeLists\.txt:[0-9]+ \(include\)$
@@ -0,0 +1 @@
+file(GLOB CONTENT_LIST RELATIVE)
@@ -0,0 +1 @@
+1
@@ -0,0 +1,4 @@
+^CMake Error at GLOB-noexp-LIST_DIRECTORIES\.cmake:[0-9]+ \(file\):
+  file GLOB requires a glob expression after the bool\.
+Call Stack \(most recent call first\):
+  CMakeLists\.txt:[0-9]+ \(include\)$
@@ -0,0 +1 @@
+1
@@ -0,0 +1,4 @@
+^CMake Error at GLOB-noexp-RELATIVE\.cmake:[0-9]+ \(file\):
+  file GLOB requires a glob expression after the directory\.
+Call Stack \(most recent call first\):
+  CMakeLists\.txt:[0-9]+ \(include\)$
@@ -0,0 +1 @@
+file(GLOB CONTENT_LIST RELATIVE "${CMAKE_CURRENT_BINARY_DIR}")
@@ -0,0 +1 @@
+1
@@ -0,0 +1,4 @@
+^CMake Error at GLOB_RECURSE-noexp-FOLLOW_SYMLINKS\.cmake:[0-9]+ \(file\):
+  file GLOB_RECURSE requires a glob expression after FOLLOW_SYMLINKS\.
+Call Stack \(most recent call first\):
+  CMakeLists\.txt:[0-9]+ \(include\)$
@@ -0,0 +1 @@
+file(GLOB_RECURSE CONTENT_LIST FOLLOW_SYMLINKS)
@@ -35,11 +35,15 @@ run_cmake(LOCK-lowercase)
 run_cmake(READ_ELF)
 run_cmake(GLOB)
 run_cmake(GLOB_RECURSE)
-# test is valid both for GLOB and GLOB_RECURSE
+run_cmake(GLOB_RECURSE-noexp-FOLLOW_SYMLINKS)
+
+# tests are valid both for GLOB and GLOB_RECURSE
+run_cmake(GLOB-error-FOLLOW_SYMLINKS)
 run_cmake(GLOB-error-LIST_DIRECTORIES-not-boolean)
-# test is valid both for GLOB and GLOB_RECURSE
 run_cmake(GLOB-error-LIST_DIRECTORIES-no-arg)
+run_cmake(GLOB-error-RELATIVE-no-arg)
 run_cmake(GLOB-noexp-LIST_DIRECTORIES)
+run_cmake(GLOB-noexp-RELATIVE)

 if(NOT WIN32 OR CYGWIN)
  run_cmake(GLOB_RECURSE-cyclic-recursion)
				`@@ -0,0 +1 @@`
				`file(GLOB CONTENT_LIST RELATIVE "${CMAKE_CURRENT_BINARY_DIR}")`
				`@@ -0,0 +1 @@`
				`file(GLOB_RECURSE CONTENT_LIST FOLLOW_SYMLINKS)`