From 372bf1bcc410f3e36397bae9834ec141a5fca7ec Mon Sep 17 00:00:00 2001 From: Robert Maynard Date: Wed, 19 May 2021 11:05:17 -0400 Subject: cmCommandLineArgument: Understands which commands require partial matching Allows us to provide better error messages when commands such as `--target` are passed invalid input. --- Source/cmCommandLineArgument.h | 48 +++++++++++++++- Source/cmake.cxx | 66 ++++++++++++++-------- Source/cmakemain.cxx | 6 +- .../build-unknown-command-partial-match-stderr.txt | 2 +- 4 files changed, 93 insertions(+), 29 deletions(-) diff --git a/Source/cmCommandLineArgument.h b/Source/cmCommandLineArgument.h index 5031c65..e269771 100644 --- a/Source/cmCommandLineArgument.h +++ b/Source/cmCommandLineArgument.h @@ -17,10 +17,17 @@ struct cmCommandLineArgument OneOrMore }; + enum class RequiresSeparator + { + Yes, + No + }; + std::string InvalidSyntaxMessage; std::string InvalidValueMessage; std::string Name; Values Type; + RequiresSeparator SeparatorNeeded; std::function StoreCall; template @@ -29,6 +36,19 @@ struct cmCommandLineArgument , InvalidValueMessage(cmStrCat("Invalid value used with ", n)) , Name(std::move(n)) , Type(t) + , SeparatorNeeded(RequiresSeparator::Yes) + , StoreCall(std::forward(func)) + { + } + + template + cmCommandLineArgument(std::string n, Values t, RequiresSeparator s, + FunctionType&& func) + : InvalidSyntaxMessage(cmStrCat(" is invalid syntax for ", n)) + , InvalidValueMessage(cmStrCat("Invalid value used with ", n)) + , Name(std::move(n)) + , Type(t) + , SeparatorNeeded(s) , StoreCall(std::forward(func)) { } @@ -40,14 +60,38 @@ struct cmCommandLineArgument , InvalidValueMessage(std::move(failedMsg)) , Name(std::move(n)) , Type(t) + , SeparatorNeeded(RequiresSeparator::Yes) + , StoreCall(std::forward(func)) + { + } + + template + cmCommandLineArgument(std::string n, std::string failedMsg, Values t, + RequiresSeparator s, FunctionType&& func) + : InvalidSyntaxMessage(cmStrCat(" is invalid syntax for ", n)) + , InvalidValueMessage(std::move(failedMsg)) + , Name(std::move(n)) + , Type(t) + , SeparatorNeeded(s) , StoreCall(std::forward(func)) { } bool matches(std::string const& input) const { - return (this->Type == Values::Zero) ? (input == this->Name) - : cmHasPrefix(input, this->Name); + if (this->Type == Values::Zero) { + return input == this->Name; + } else if (this->SeparatorNeeded == RequiresSeparator::No) { + return cmHasPrefix(input, this->Name); + } else if (cmHasPrefix(input, this->Name)) { + if (input.size() == this->Name.size()) { + return true; + } else { + return (input[this->Name.size()] == '=' || + input[this->Name.size()] == ' '); + } + } + return false; } template diff --git a/Source/cmake.cxx b/Source/cmake.cxx index 5440984..315bd20 100644 --- a/Source/cmake.cxx +++ b/Source/cmake.cxx @@ -529,25 +529,29 @@ bool cmake::SetCacheArgs(const std::vector& args) std::vector arguments = { CommandArgument{ "-D", "-D must be followed with VAR=VALUE.", - CommandArgument::Values::One, DefineLambda }, + CommandArgument::Values::One, + CommandArgument::RequiresSeparator::No, DefineLambda }, CommandArgument{ "-W", "-W must be followed with [no-].", - CommandArgument::Values::One, WarningLambda }, + CommandArgument::Values::One, + CommandArgument::RequiresSeparator::No, WarningLambda }, CommandArgument{ "-U", "-U must be followed with VAR.", - CommandArgument::Values::One, UnSetLambda }, - CommandArgument{ "-C", "-C must be followed by a file name.", CommandArgument::Values::One, - [&](std::string const& value, cmake* state) -> bool { - cmSystemTools::Stdout("loading initial cache file " + - value + "\n"); - // Resolve script path specified on command line - // relative to $PWD. - auto path = cmSystemTools::CollapseFullPath(value); - state->ReadListFile(args, path); - return true; - } }, + CommandArgument::RequiresSeparator::No, UnSetLambda }, + CommandArgument{ + "-C", "-C must be followed by a file name.", + CommandArgument::Values::One, CommandArgument::RequiresSeparator::No, + [&](std::string const& value, cmake* state) -> bool { + cmSystemTools::Stdout("loading initial cache file " + value + "\n"); + // Resolve script path specified on command line + // relative to $PWD. + auto path = cmSystemTools::CollapseFullPath(value); + state->ReadListFile(args, path); + return true; + } }, CommandArgument{ "-P", "-P must be followed by a file name.", - CommandArgument::Values::One, ScriptLambda }, + CommandArgument::Values::One, + CommandArgument::RequiresSeparator::No, ScriptLambda }, CommandArgument{ "--toolchain", "No file specified for --toolchain", CommandArgument::Values::One, ToolchainLambda }, CommandArgument{ "--install-prefix", @@ -830,31 +834,44 @@ void cmake::SetArgs(const std::vector& args) std::vector arguments = { CommandArgument{ "-S", "No source directory specified for -S", - CommandArgument::Values::One, SourceArgLambda }, + CommandArgument::Values::One, + CommandArgument::RequiresSeparator::No, SourceArgLambda }, CommandArgument{ "-H", "No source directory specified for -H", - CommandArgument::Values::One, SourceArgLambda }, + CommandArgument::Values::One, + CommandArgument::RequiresSeparator::No, SourceArgLambda }, CommandArgument{ "-O", CommandArgument::Values::Zero, IgnoreAndTrueLambda }, CommandArgument{ "-B", "No build directory specified for -B", - CommandArgument::Values::One, BuildArgLambda }, + CommandArgument::Values::One, + CommandArgument::RequiresSeparator::No, BuildArgLambda }, CommandArgument{ "-P", "-P must be followed by a file name.", CommandArgument::Values::One, + CommandArgument::RequiresSeparator::No, [&](std::string const&, cmake*) -> bool { scriptMode = true; return true; } }, CommandArgument{ "-D", "-D must be followed with VAR=VALUE.", - CommandArgument::Values::One, IgnoreAndTrueLambda }, + CommandArgument::Values::One, + CommandArgument::RequiresSeparator::No, + IgnoreAndTrueLambda }, CommandArgument{ "-C", "-C must be followed by a file name.", - CommandArgument::Values::One, IgnoreAndTrueLambda }, - CommandArgument{ "-U", "-U must be followed with VAR.", - CommandArgument::Values::One, IgnoreAndTrueLambda }, + CommandArgument::Values::One, + CommandArgument::RequiresSeparator::No, + IgnoreAndTrueLambda }, + CommandArgument{ + "-U", "-U must be followed with VAR.", CommandArgument::Values::One, + CommandArgument::RequiresSeparator::No, IgnoreAndTrueLambda }, CommandArgument{ "-W", "-W must be followed with [no-].", - CommandArgument::Values::One, IgnoreAndTrueLambda }, + CommandArgument::Values::One, + CommandArgument::RequiresSeparator::No, + IgnoreAndTrueLambda }, CommandArgument{ "-A", "No platform specified for -A", - CommandArgument::Values::One, PlatformLambda }, + CommandArgument::Values::One, + CommandArgument::RequiresSeparator::No, PlatformLambda }, CommandArgument{ "-T", "No toolset specified for -T", - CommandArgument::Values::One, ToolsetLamda }, + CommandArgument::Values::One, + CommandArgument::RequiresSeparator::No, ToolsetLamda }, CommandArgument{ "--toolchain", "No file specified for --toolchain", CommandArgument::Values::One, IgnoreAndTrueLambda }, CommandArgument{ "--install-prefix", @@ -1079,6 +1096,7 @@ void cmake::SetArgs(const std::vector& args) bool badGeneratorName = false; CommandArgument generatorCommand( "-G", "No generator specified for -G", CommandArgument::Values::One, + CommandArgument::RequiresSeparator::No, [&](std::string const& value, cmake* state) -> bool { bool valid = state->CreateAndSetGlobalGenerator(value, true); badGeneratorName = !valid; diff --git a/Source/cmakemain.cxx b/Source/cmakemain.cxx index ad64818..997d855 100644 --- a/Source/cmakemain.cxx +++ b/Source/cmakemain.cxx @@ -271,6 +271,7 @@ int do_cmake(int ac, char const* const* av) } }, CommandArgument{ "-P", "No script specified for argument -P", CommandArgument::Values::One, + CommandArgument::RequiresSeparator::No, [&](std::string const& value) -> bool { workingMode = cmake::SCRIPT_MODE; parsedArgs.emplace_back("-P"); @@ -476,9 +477,10 @@ int do_build(int ac, char const* const* av) listPresets = true; return true; } }, - CommandArgument{ "-j", CommandArgument::Values::ZeroOrOne, jLambda }, + CommandArgument{ "-j", CommandArgument::Values::ZeroOrOne, + CommandArgument::RequiresSeparator::No, jLambda }, CommandArgument{ "--parallel", CommandArgument::Values::ZeroOrOne, - parallelLambda }, + CommandArgument::RequiresSeparator::No, parallelLambda }, CommandArgument{ "-t", CommandArgument::Values::OneOrMore, targetLambda }, CommandArgument{ "--target", CommandArgument::Values::OneOrMore, targetLambda }, diff --git a/Tests/RunCMake/CommandLine/build-unknown-command-partial-match-stderr.txt b/Tests/RunCMake/CommandLine/build-unknown-command-partial-match-stderr.txt index d69338a..e4ad6b2 100644 --- a/Tests/RunCMake/CommandLine/build-unknown-command-partial-match-stderr.txt +++ b/Tests/RunCMake/CommandLine/build-unknown-command-partial-match-stderr.txt @@ -1,2 +1,2 @@ -^CMake Error: '--targetinvalid' is invalid syntax for --target +^Unknown argument --targetinvalid Usage: cmake --build \[ \| --preset \] \[options\] \[-- \[native-options\]\] -- cgit v0.12 From 396e0a840e75afb3f5ed7b50892b93ce946fac2e Mon Sep 17 00:00:00 2001 From: Robert Maynard Date: Wed, 19 May 2021 11:07:34 -0400 Subject: cmCommandLineArgument: OneOrMore mode supports `=` separator Fixes #22187 --- Source/cmCommandLineArgument.h | 75 +++++++++++++--------- .../build-invalid-target-syntax-stderr.txt | 3 +- 2 files changed, 46 insertions(+), 32 deletions(-) diff --git a/Source/cmCommandLineArgument.h b/Source/cmCommandLineArgument.h index e269771..72ab045 100644 --- a/Source/cmCommandLineArgument.h +++ b/Source/cmCommandLineArgument.h @@ -23,6 +23,14 @@ struct cmCommandLineArgument No }; + enum class ParseMode + { + Valid, + Invalid, + SyntaxError, + ValueError + }; + std::string InvalidSyntaxMessage; std::string InvalidValueMessage; std::string Name; @@ -79,19 +87,20 @@ struct cmCommandLineArgument bool matches(std::string const& input) const { + bool matched = false; if (this->Type == Values::Zero) { - return input == this->Name; + matched = (input == this->Name); } else if (this->SeparatorNeeded == RequiresSeparator::No) { - return cmHasPrefix(input, this->Name); + matched = cmHasPrefix(input, this->Name); } else if (cmHasPrefix(input, this->Name)) { if (input.size() == this->Name.size()) { - return true; + matched = true; } else { - return (input[this->Name.size()] == '=' || - input[this->Name.size()] == ' '); + matched = + (input[this->Name.size()] == '=' || input[this->Name.size()] == ' '); } } - return false; + return matched; } template @@ -99,13 +108,6 @@ struct cmCommandLineArgument std::vector const& allArgs, CallState&&... state) const { - enum class ParseMode - { - Valid, - Invalid, - SyntaxError, - ValueError - }; ParseMode parseState = ParseMode::Valid; if (this->Type == Values::Zero) { @@ -139,23 +141,10 @@ struct cmCommandLineArgument index = nextValueIndex; } } else { - // parse the string to get the value - auto possible_value = cm::string_view(input).substr(this->Name.size()); - if (possible_value.empty()) { - parseState = ParseMode::ValueError; - } else if (possible_value[0] == '=') { - possible_value.remove_prefix(1); - if (possible_value.empty()) { - parseState = ParseMode::ValueError; - } - } + auto value = this->extract_single_value(input, parseState); if (parseState == ParseMode::Valid) { - if (possible_value[0] == ' ') { - possible_value.remove_prefix(1); - } - - parseState = this->StoreCall(std::string(possible_value), - std::forward(state)...) + parseState = + this->StoreCall(value, std::forward(state)...) ? ParseMode::Valid : ParseMode::Invalid; } @@ -193,7 +182,13 @@ struct cmCommandLineArgument index = (nextValueIndex - 1); } } else { - parseState = ParseMode::SyntaxError; + auto value = this->extract_single_value(input, parseState); + if (parseState == ParseMode::Valid) { + parseState = + this->StoreCall(value, std::forward(state)...) + ? ParseMode::Valid + : ParseMode::Invalid; + } } } @@ -205,4 +200,24 @@ struct cmCommandLineArgument } return (parseState == ParseMode::Valid); } + +private: + std::string extract_single_value(std::string const& input, + ParseMode& parseState) const + { + // parse the string to get the value + auto possible_value = cm::string_view(input).substr(this->Name.size()); + if (possible_value.empty()) { + parseState = ParseMode::ValueError; + } else if (possible_value[0] == '=') { + possible_value.remove_prefix(1); + if (possible_value.empty()) { + parseState = ParseMode::ValueError; + } + } + if (parseState == ParseMode::Valid && possible_value[0] == ' ') { + possible_value.remove_prefix(1); + } + return std::string(possible_value); + } }; diff --git a/Tests/RunCMake/CommandLine/build-invalid-target-syntax-stderr.txt b/Tests/RunCMake/CommandLine/build-invalid-target-syntax-stderr.txt index 5fe2539..fa3adc8 100644 --- a/Tests/RunCMake/CommandLine/build-invalid-target-syntax-stderr.txt +++ b/Tests/RunCMake/CommandLine/build-invalid-target-syntax-stderr.txt @@ -1,2 +1 @@ -^CMake Error: '--target=invalid' is invalid syntax for --target -Usage: cmake --build \[ \| --preset \] \[options\] \[-- \[native-options\]\] +^Error: could not load cache -- cgit v0.12