source_group: use cmArgumentParser

To facilite any future evolutions, use cmArgumentParser to
parse method arguments.

Source/cmMakefile.cxx

@@ -2186,7 +2186,7 @@ cmSourceGroup* cmMakefile::GetSourceGroup(
   return sg;
 }
 
-void cmMakefile::AddSourceGroup(std::string const& name, char const* regex)
+void cmMakefile::AddSourceGroup(std::string const& name, cm::string_view regex)
 {
   std::vector<std::string> nameVector;
   nameVector.push_back(name);
@@ -2194,7 +2194,7 @@ void cmMakefile::AddSourceGroup(std::string const& name, char const* regex)
 }
 
 void cmMakefile::AddSourceGroup(std::vector<std::string> const& name,
-                                char const* regex)
+                                cm::string_view regex)
 {
   cmSourceGroup* sg = nullptr;
   std::vector<std::string> currentName;
@@ -2211,7 +2211,7 @@ void cmMakefile::AddSourceGroup(std::vector<std::string> const& name,
   // i now contains the index of the last found component
   if (i == lastElement) {
     // group already exists, replace its regular expression
-    if (regex && sg) {
+    if (regex.data() && sg) {
       // We only want to set the regular expression.  If there are already
       // source files in the group, we don't want to remove them.
       sg->SetGroupRegex(regex);
@@ -2232,8 +2232,8 @@ void cmMakefile::AddSourceGroup(std::vector<std::string> const& name,
   }
   // build the whole source group path
   for (++i; i <= lastElement; ++i) {
-    sg->AddChild(cm::make_unique<cmSourceGroup>(name[i], nullptr,
-                                                sg->GetFullName().c_str()));
+    sg->AddChild(cm::make_unique<cmSourceGroup>(name[i], cm::string_view{},
+                                                sg->GetFullName()));
     sg = sg->LookupChild(name[i]);
   }
 

Source/cmMakefile.h

@@ -682,14 +682,14 @@ public:
   /**
    * Add a root source group for consideration when adding a new source.
    */
-  void AddSourceGroup(std::string const& name, char const* regex = nullptr);
+  void AddSourceGroup(std::string const& name, cm::string_view regex = {});
 
   /**
    * Add a source group for consideration when adding a new source.
    * name is tokenized.
    */
   void AddSourceGroup(std::vector<std::string> const& name,
-                      char const* regex = nullptr);
+                      cm::string_view regex = {});
 
   /**
    * Get and existing or create a new source group.

Source/cmSourceGroup.cxx

@@ -15,27 +15,27 @@ public:
   SourceGroupVector GroupChildren;
 };
 
-cmSourceGroup::cmSourceGroup(std::string name, char const* regex,
-                             char const* parentName)
+cmSourceGroup::cmSourceGroup(std::string name, cm::string_view regex,
+                             cm::string_view parentName)
   : Name(std::move(name))
 {
   this->Internal = cm::make_unique<cmSourceGroupInternals>();
   this->SetGroupRegex(regex);
-  if (parentName) {
-    this->FullName = cmStrCat(parentName, '\\');
+  if (parentName.empty()) {
+    this->FullName = this->Name;
+  } else {
+    this->FullName = cmStrCat(parentName, '\\', this->Name);
   }
-  this->FullName += this->Name;
 }
 
 cmSourceGroup::~cmSourceGroup() = default;
 
-void cmSourceGroup::SetGroupRegex(char const* regex)
+bool cmSourceGroup::SetGroupRegex(cm::string_view regex)
 {
-  if (regex) {
-    this->GroupRegex.compile(regex);
-  } else {
-    this->GroupRegex.compile("^$");
+  if (regex.data()) {
+    return this->GroupRegex.compile(static_cast<std::string>(regex));
   }
+  return this->GroupRegex.compile("^$");
 }
 
 void cmSourceGroup::ResolveGenex(cmLocalGenerator* lg,

Source/cmSourceGroup.h

@@ -10,6 +10,8 @@
 #include <string>
 #include <vector>
 
+#include <cm/string_view>
+
 #include "cmsys/RegularExpression.hxx"
 
 class cmLocalGenerator;
@@ -32,16 +34,17 @@ using SourceGroupVector = std::vector<std::unique_ptr<cmSourceGroup>>;
 class cmSourceGroup
 {
 public:
-  cmSourceGroup(std::string name, char const* regex,
-                char const* parentName = nullptr);
+  cmSourceGroup(std::string name, cm::string_view regex,
+                cm::string_view parentName = {});
   cmSourceGroup(cmSourceGroup const& r) = delete;
   ~cmSourceGroup();
   cmSourceGroup& operator=(cmSourceGroup const&) = delete;
 
   /**
    * Set the regular expression for this group.
+   * Returns false if the regular expression cannot be compiled.
    */
-  void SetGroupRegex(char const* regex);
+  bool SetGroupRegex(cm::string_view regex);
 
   /**
    * Resolve genex.

Source/cmSourceGroupCommand.cxx

@@ -9,7 +9,12 @@
 #include <utility>
 
 #include <cmext/algorithm>
+#include <cmext/string_view>
 
+#include "cmArgumentParser.h"
+#include "cmArgumentParserTypes.h"
+#include "cmCMakePath.h"
+#include "cmDiagnostics.h"
 #include "cmExecutionStatus.h"
 #include "cmMakefile.h"
 #include "cmSourceFile.h"
@@ -19,164 +24,80 @@
 #include "cmSystemTools.h"
 
 namespace {
-
-using ParsedArguments = std::map<std::string, std::vector<std::string>>;
-using ExpectedOptions = std::vector<std::string>;
-
-std::string const kTreeOptionName = "TREE";
-std::string const kPrefixOptionName = "PREFIX";
-std::string const kFilesOptionName = "FILES";
-std::string const kRegexOptionName = "REGULAR_EXPRESSION";
-std::string const kSourceGroupOptionName = "<sg_name>";
-
-std::set<std::string> getSourceGroupFilesPaths(
-  std::string const& root, std::vector<std::string> const& files)
+template <typename Args>
+bool ProcessTree(Args const& args, cmExecutionStatus& status)
 {
-  std::set<std::string> ret;
-  std::string::size_type const rootLength = root.length();
+  cmMakefile& mf = status.GetMakefile();
 
-  for (std::string const& file : files) {
-    ret.insert(file.substr(rootLength + 1)); // +1 to also omnit last '/'
+  auto const& currentSourceDir = mf.GetCurrentSourceDirectory();
+  std::vector<std::string> files;
+  if (args.Files) {
+    files.reserve(args.Files->size());
+
+    for (auto const& file : *args.Files) {
+      if (file.empty()) {
+        continue;
+      }
+      std::string fullPath =
+        cmSystemTools::CollapseFullPath(file, currentSourceDir);
+      files.emplace_back(std::move(fullPath));
+    }
+  } else {
+    std::vector<std::unique_ptr<cmSourceFile>> const& sources =
+      mf.GetSourceFiles();
+    for (auto const& src : sources) {
+      if (!src->GetIsGenerated()) {
+        files.push_back(cmSystemTools::CollapseFullPath(
+          src->GetLocation().GetFullPath(), currentSourceDir));
+      }
+    }
   }
 
-  return ret;
-}
+  // final checks
+  cmCMakePath const root{ cmSystemTools::CollapseFullPath(*args.Tree) };
+  cmCMakePath const prefix{
+    cmCMakePath{ args.Prefix ? *args.Prefix : "" }.Normal()
+  };
 
-bool rootIsPrefix(std::string const& root,
-                  std::vector<std::string> const& files, std::string& error)
-{
-  for (std::string const& file : files) {
-    if (!cmHasPrefix(file, root)) {
-      error = cmStrCat("ROOT: ", root, " is not a prefix of file: ", file);
+  auto it = files.begin();
+  while (it != files.end()) {
+    if (it->empty() || cmSystemTools::FileIsDirectory(*it) ||
+        (it->back() == '/' || it->back() == '\\')) {
+      // Ignore any empty files or directories
+      it = files.erase(it);
+      continue;
+    }
+
+    cmCMakePath file{ *it };
+    if (!root.IsPrefix(file)) {
+      status.SetError(cmStrCat('"', root.GenericString(),
+                               "\" is not a prefix of file \"",
+                               file.GenericString(), '"'));
       return false;
     }
-  }
 
-  return true;
-}
-
-std::vector<std::string> prepareFilesPathsForTree(
-  std::vector<std::string> const& filesPaths,
-  std::string const& currentSourceDir)
-{
-  std::vector<std::string> prepared;
-  prepared.reserve(filesPaths.size());
-
-  for (auto const& filePath : filesPaths) {
-    std::string fullPath =
-      cmSystemTools::CollapseFullPath(filePath, currentSourceDir);
-    // If provided file path is actually not a directory, silently ignore it.
-    if (cmSystemTools::FileIsDirectory(fullPath)) {
-      continue;
+    // source groups generation
+    cmCMakePath sourceGroup = prefix / file.Relative(root);
+    if (sourceGroup.HasParentPath()) {
+      sourceGroup = sourceGroup.GetParentPath();
     }
+    std::vector<std::string> tokenizedSG =
+      cmTokenize(sourceGroup.GenericString(), '/', cmTokenizerMode::New);
 
-    // Handle directory that doesn't exist yet.
-    if (!fullPath.empty() &&
-        (fullPath.back() == '/' || fullPath.back() == '\\')) {
-      continue;
-    }
-
-    prepared.emplace_back(std::move(fullPath));
-  }
-
-  return prepared;
-}
-
-bool addFilesToItsSourceGroups(std::string const& root,
-                               std::set<std::string> const& sgFilesPaths,
-                               std::string const& prefix, cmMakefile& makefile,
-                               std::string& errorMsg)
-{
-  cmSourceGroup* sg;
-
-  for (std::string const& sgFilesPath : sgFilesPaths) {
-    std::vector<std::string> tokenizedPath = cmTokenize(
-      prefix.empty() ? sgFilesPath : cmStrCat(prefix, '/', sgFilesPath),
-      R"(\/)", cmTokenizerMode::New);
-
-    if (tokenizedPath.empty()) {
-      continue;
-    }
-    tokenizedPath.pop_back();
-
-    if (tokenizedPath.empty()) {
-      tokenizedPath.emplace_back();
-    }
-
-    sg = makefile.GetOrCreateSourceGroup(tokenizedPath);
-
+    auto* sg = mf.GetOrCreateSourceGroup(tokenizedSG);
     if (!sg) {
-      errorMsg = "Could not create source group for file: " + sgFilesPath;
+      status.SetError(cmStrCat("could not create source group for file \"",
+                               file.GenericString(), '"'));
       return false;
     }
-    std::string const fullPath =
-      cmSystemTools::CollapseFullPath(sgFilesPath, root);
-    sg->AddGroupFile(fullPath);
-  }
+    sg->AddGroupFile(file.GenericString());
 
+    ++it;
+  }
   return true;
 }
-
-ExpectedOptions getExpectedOptions()
-{
-  ExpectedOptions options;
-
-  options.push_back(kTreeOptionName);
-  options.push_back(kPrefixOptionName);
-  options.push_back(kFilesOptionName);
-  options.push_back(kRegexOptionName);
-
-  return options;
-}
-
-bool isExpectedOption(std::string const& argument,
-                      ExpectedOptions const& expectedOptions)
-{
-  return cm::contains(expectedOptions, argument);
-}
-
-void parseArguments(std::vector<std::string> const& args,
-                    ParsedArguments& parsedArguments)
-{
-  ExpectedOptions const expectedOptions = getExpectedOptions();
-  size_t i = 0;
-
-  // at this point we know that args vector is not empty
-
-  // if first argument is not one of expected options it's source group name
-  if (!isExpectedOption(args[0], expectedOptions)) {
-    // get source group name and go to next argument
-    parsedArguments[kSourceGroupOptionName].push_back(args[0]);
-    ++i;
-  }
-
-  for (; i < args.size();) {
-    // get current option and increment index to go to next argument
-    std::string const& currentOption = args[i++];
-
-    // create current option entry in parsed arguments
-    std::vector<std::string>& currentOptionArguments =
-      parsedArguments[currentOption];
-
-    // collect option arguments while we won't find another expected option
-    while (i < args.size() && !isExpectedOption(args[i], expectedOptions)) {
-      currentOptionArguments.push_back(args[i++]);
-    }
-  }
-}
-
 } // namespace
 
-static bool checkArgumentsPreconditions(ParsedArguments const& parsedArguments,
-                                        std::string& errorMsg);
-
-static bool processTree(cmMakefile& mf, ParsedArguments& parsedArguments,
-                        std::string& errorMsg);
-
-static bool checkSingleParameterArgumentPreconditions(
-  std::string const& argument, ParsedArguments const& parsedArguments,
-  std::string& errorMsg);
-
 bool cmSourceGroupCommand(std::vector<std::string> const& args,
                           cmExecutionStatus& status)
 {
@@ -185,138 +106,111 @@ bool cmSourceGroupCommand(std::vector<std::string> const& args,
     return false;
   }
 
+  static cm::string_view const Keywords[]{ "TREE"_s, "PREFIX"_s, "FILES"_s,
+                                           "REGULAR_EXPRESSION"_s };
   cmMakefile& mf = status.GetMakefile();
 
-  // If only two arguments are given, the pre-1.8 version of the
-  // command is being invoked.
-  bool isShortTreeSyntax =
-    ((args.size() == 2) && (args[0] == kTreeOptionName) &&
-     cmSystemTools::FileIsDirectory(args[1]));
-  if (args.size() == 2 && args[1] != kFilesOptionName && !isShortTreeSyntax) {
+  if (args.size() == 2 && !cm::contains(Keywords, args[0]) &&
+      !cm::contains(Keywords, args[1])) {
+    // The pre-1.8 version of the command is being invoked.
     cmSourceGroup* sg = mf.GetOrCreateSourceGroup(args[0]);
-
     if (!sg) {
-      status.SetError("Could not create or find source group");
+      status.SetError("Could not create or find source group.");
       return false;
     }
-
-    sg->SetGroupRegex(args[1].c_str());
+    sg->SetGroupRegex(args[1]);
     return true;
   }
 
-  ParsedArguments parsedArguments;
-  std::string errorMsg;
+  struct Arguments : public ArgumentParser::ParseResult
+  {
+    cm::optional<std::string> GroupName;
+    cm::optional<ArgumentParser::MaybeEmpty<std::vector<std::string>>> Files;
+    cm::optional<std::string> Regex;
+    cm::optional<ArgumentParser::NonEmpty<std::string>> Tree;
+    cm::optional<ArgumentParser::MaybeEmpty<std::string>> Prefix;
+  };
 
-  parseArguments(args, parsedArguments);
+  auto unsupportedKeyword =
+    [&mf](Arguments&, cm::string_view key,
+          cm::string_view /*value */) -> ArgumentParser::Continue {
+    mf.IssueDiagnostic(
+      cmDiagnostics::CMD_AUTHOR,
+      cmStrCat("keyword \"", key, "\" will be ignored in this context."));
+    return ArgumentParser::Continue::Yes;
+  };
+  // to distinguish REGULAR_EXPRESSION without values from with an empty string
+  auto handleRegex = [](Arguments& result,
+                        cm::string_view value) -> ArgumentParser::Continue {
+    if (value.data()) {
+      result.Regex = std::string{ value };
+    }
+    return ArgumentParser::Continue::No;
+  };
 
-  if (!checkArgumentsPreconditions(parsedArguments, errorMsg)) {
+  std::vector<std::string> unexpectedArgs;
+  auto parser =
+    cmArgumentParser<Arguments>{}.Bind("FILES"_s, &Arguments::Files);
+
+  if (cm::contains(args, "TREE")) {
+    // this is the TREE syntax
+    parser.Bind("TREE"_s, &Arguments::Tree)
+      .Bind("PREFIX"_s, &Arguments::Prefix)
+      .Bind("REGULAR_EXPRESSION"_s, unsupportedKeyword);
+  } else {
+    // assume that first argument is the group name
+    parser.Bind(0, &Arguments::GroupName)
+      .Bind("REGULAR_EXPRESSION"_s, handleRegex, 0)
+      .Bind("TREE"_s, unsupportedKeyword)
+      .Bind("PREFIX"_s, unsupportedKeyword);
+  }
+
+  auto parsedArgs = parser.Parse(args, &unexpectedArgs);
+
+  // do various checks for arguments consistency
+  if (!parsedArgs.Check("", &unexpectedArgs, status)) {
+    cmSystemTools::SetFatalErrorOccurred();
+    return false;
+  }
+  if (!parsedArgs.Tree && !parsedArgs.GroupName) {
+    status.SetError("missing source group name.");
+    cmSystemTools::SetFatalErrorOccurred();
     return false;
   }
 
-  if (parsedArguments.find(kTreeOptionName) != parsedArguments.end()) {
-    if (!processTree(mf, parsedArguments, errorMsg)) {
-      status.SetError(errorMsg);
+  if (parsedArgs.Tree) {
+    if (!ProcessTree(parsedArgs, status)) {
+      cmSystemTools::SetFatalErrorOccurred();
       return false;
     }
   } else {
-    if (parsedArguments.find(kSourceGroupOptionName) ==
-        parsedArguments.end()) {
-      status.SetError("Missing source group name.");
-      return false;
+    if (!parsedArgs.Files && !parsedArgs.Regex) {
+      // group is not created
+      return true;
     }
 
-    cmSourceGroup* sg = mf.GetOrCreateSourceGroup(args[0]);
-
+    auto* sg = mf.GetOrCreateSourceGroup(*parsedArgs.GroupName);
     if (!sg) {
-      status.SetError("Could not create or find source group");
+      status.SetError("could not create or find source group");
+      cmSystemTools::SetFatalErrorOccurred();
       return false;
     }
 
-    // handle regex
-    if (parsedArguments.find(kRegexOptionName) != parsedArguments.end()) {
-      std::string const& sgRegex = parsedArguments[kRegexOptionName].front();
-      sg->SetGroupRegex(sgRegex.c_str());
+    if (parsedArgs.Regex) {
+      if (!sg->SetGroupRegex(*parsedArgs.Regex)) {
+        status.SetError(cmStrCat("regular expression \"", *parsedArgs.Regex,
+                                 "\" is invalid"));
+        return false;
+      }
     }
-
-    // handle files
-    std::vector<std::string> const& filesArguments =
-      parsedArguments[kFilesOptionName];
-    for (auto const& filesArg : filesArguments) {
-      std::string src = filesArg;
-      src =
-        cmSystemTools::CollapseFullPath(src, mf.GetCurrentSourceDirectory());
-      sg->AddGroupFile(src);
-    }
-  }
-
-  return true;
-}
-
-static bool checkArgumentsPreconditions(ParsedArguments const& parsedArguments,
-                                        std::string& errorMsg)
-{
-  return checkSingleParameterArgumentPreconditions(
-           kPrefixOptionName, parsedArguments, errorMsg) &&
-    checkSingleParameterArgumentPreconditions(kTreeOptionName, parsedArguments,
-                                              errorMsg) &&
-    checkSingleParameterArgumentPreconditions(kRegexOptionName,
-                                              parsedArguments, errorMsg);
-}
-
-static bool processTree(cmMakefile& mf, ParsedArguments& parsedArguments,
-                        std::string& errorMsg)
-{
-  std::string const root =
-    cmSystemTools::CollapseFullPath(parsedArguments[kTreeOptionName].front());
-  std::string prefix = parsedArguments[kPrefixOptionName].empty()
-    ? ""
-    : parsedArguments[kPrefixOptionName].front();
-
-  std::vector<std::string> files;
-  auto filesArgIt = parsedArguments.find(kFilesOptionName);
-  if (filesArgIt != parsedArguments.end()) {
-    files = filesArgIt->second;
-  } else {
-    std::vector<std::unique_ptr<cmSourceFile>> const& srcFiles =
-      mf.GetSourceFiles();
-    for (auto const& srcFile : srcFiles) {
-      if (!srcFile->GetIsGenerated()) {
-        files.push_back(srcFile->GetLocation().GetFullPath());
+    if (parsedArgs.Files) {
+      auto const& currentSourceDir = mf.GetCurrentSourceDirectory();
+      for (auto const& file : *parsedArgs.Files) {
+        sg->AddGroupFile(
+          cmSystemTools::CollapseFullPath(file, currentSourceDir));
       }
     }
   }
 
-  std::vector<std::string> const filesVector =
-    prepareFilesPathsForTree(files, mf.GetCurrentSourceDirectory());
-
-  if (!rootIsPrefix(root, filesVector, errorMsg)) {
-    return false;
-  }
-
-  std::set<std::string> sourceGroupPaths =
-    getSourceGroupFilesPaths(root, filesVector);
-
-  return addFilesToItsSourceGroups(root, sourceGroupPaths, prefix, mf,
-                                   errorMsg);
-}
-
-static bool checkSingleParameterArgumentPreconditions(
-  std::string const& argument, ParsedArguments const& parsedArguments,
-  std::string& errorMsg)
-{
-  auto foundArgument = parsedArguments.find(argument);
-  if (foundArgument != parsedArguments.end()) {
-    std::vector<std::string> const& optionArguments = foundArgument->second;
-
-    if (optionArguments.empty()) {
-      errorMsg = argument + " argument given without an argument.";
-      return false;
-    }
-    if (optionArguments.size() > 1) {
-      errorMsg = "too many arguments passed to " + argument + ".";
-      return false;
-    }
-  }
-
   return true;
 }

Tests/RunCMake/CMakeLists.txt

@@ -878,6 +878,7 @@ add_RunCMake_test(alias_targets)
 add_RunCMake_test(InterfaceLibrary)
 add_RunCMake_test(IntermediateDirStrategy)
 add_RunCMake_test(no_install_prefix)
+add_RunCMake_test(SourceGroup)
 add_RunCMake_test(configure_file)
 if(CTestTestTimeout_TIME)
   set(CTestTimeout_ARGS -DTIMEOUT=${CTestTestTimeout_TIME})

Tests/RunCMake/SourceGroup/CMakeLists.txt

@@ -0,0 +1,3 @@
+cmake_minimum_required(VERSION 4.3...4.4)
+project(${RunCMake_TEST} LANGUAGES NONE)
+include(${RunCMake_TEST}.cmake NO_POLICY_SCOPE)

Tests/RunCMake/SourceGroup/IgnoredKeywords-stderr.txt

@@ -0,0 +1,11 @@
+CMake Warning \(author\) at IgnoredKeywords\.cmake:[0-9]+ \(source_group\):
+  keyword "PREFIX" will be ignored in this context\.
+Call Stack \(most recent call first\):
+  CMakeLists\.txt:[0-9]+ \(include\)
+This warning is for project developers\.  Use -Wno-author to suppress it\.
+
+CMake Warning \(author\) at IgnoredKeywords\.cmake:[0-9]+ \(source_group\):
+  keyword "REGULAR_EXPRESSION" will be ignored in this context\.
+Call Stack \(most recent call first\):
+  CMakeLists\.txt:[0-9]+ \(include\)
+This warning is for project developers\.  Use -Wno-author to suppress it\.

Tests/RunCMake/SourceGroup/IgnoredKeywords.cmake

@@ -0,0 +1,4 @@
+
+source_group(foo PREFIX bar)
+
+source_group(TREE foo REGULAR_EXPRESSION ".*")

Tests/RunCMake/SourceGroup/MixedSignatures-result.txt

@@ -0,0 +1 @@
+1

Tests/RunCMake/SourceGroup/MixedSignatures-stderr.txt

@@ -0,0 +1,4 @@
+CMake Error at MixedSignatures\.cmake:[0-9]+ \(source_group\):
+  source_group given unknown argument: "foo".
+Call Stack \(most recent call first\):
+  CMakeLists\.txt:[0-9]+ \(include\)

Tests/RunCMake/SourceGroup/MixedSignatures.cmake

@@ -0,0 +1,2 @@
+
+source_group(foo TREE bar)

Tests/RunCMake/SourceGroup/RunCMakeTest.cmake

@@ -0,0 +1,6 @@
+
+include(RunCMake)
+
+run_cmake(MixedSignatures)
+run_cmake(WrongRegex)
+run_cmake(IgnoredKeywords)

Tests/RunCMake/SourceGroup/WrongRegex-result.txt

@@ -0,0 +1 @@
+1

Tests/RunCMake/SourceGroup/WrongRegex-stderr.txt

@@ -0,0 +1,4 @@
+CMake Error at WrongRegex\.cmake:[0-9]+ \(source_group\):
+  source_group regular expression "\(" is invalid
+Call Stack \(most recent call first\):
+  CMakeLists\.txt:[0-9]+ \(include\)

Tests/RunCMake/SourceGroup/WrongRegex.cmake

@@ -0,0 +1,2 @@
+
+source_group(foo REGULAR_EXPRESSION "(")