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

4 files changed, 104 insertions, 103 deletions

diff --git a/Source/cmCacheManager.cxx b/Source/cmCacheManager.cxx
index fab2445..85ac985 100644
--- a/Source/cmCacheManager.cxx
+++ b/Source/cmCacheManager.cxx
@@ -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;
diff --git a/Source/cmFileCommand.cxx b/Source/cmFileCommand.cxx
index 8492c17..b5215a5 100644
--- a/Source/cmFileCommand.cxx
+++ b/Source/cmFileCommand.cxx
@@ -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;
+
+      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;
+        }
       }
-    }
 
-    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 += ";";
+      if (recurse && !explicitFollowSymlinks &&
+          g.GetFollowedSymlinkCount() != 0) {
+        warnFollowedSymlinks = true;
       }
-      output += files[cc];
-      first = false;
-    }
-  }
 
-  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));
+      std::vector<std::string>& files = g.GetFiles();
+      std::sort(files.begin(), files.end());
+      for (std::string const& file : files) {
+        if (!first) {
+          outputStream << ";";
         }
-        break;
+        outputStream << file;
+        first = false;
+      }
+      ++i;
     }
   }
 
-  this->Makefile->AddDefinition(variable, output.c_str());
+  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, outputStream.str().c_str());
   return true;
 }
 
diff --git a/Source/cmFileTimeComparison.h b/Source/cmFileTimeComparison.h
index b1f8ed6..114989b 100644
--- a/Source/cmFileTimeComparison.h
+++ b/Source/cmFileTimeComparison.h
@@ -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
 {
diff --git a/Source/cmGlobalXCodeGenerator.cxx b/Source/cmGlobalXCodeGenerator.cxx
index 0d5dad2..7fdf896 100644
--- a/Source/cmGlobalXCodeGenerator.cxx
+++ b/Source/cmGlobalXCodeGenerator.cxx
@@ -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";
           }
         }