<div dir="ltr"><div dir="ltr"><div>Hi Thomas, Paul,<br></div><div><br></div><div></div><div class="gmail_quote"><div dir="ltr">On Mon, Jan 14, 2019 at 1:14 PM Thomas Preud'homme via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: thopre<br>
Date: Mon Jan 14 01:28:53 2019<br>
New Revision: 351038<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=351038&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=351038&view=rev</a><br>
Log:<br>
Add support for prefix-only CLI options<br>
<br>
Summary:<br>
Add support for options that always prefix their value, giving an error<br>
if the value is in the next argument or if the option is given a value<br>
assignment (ie. opt=val). This is the desired behavior for the -D option<br>
of FileCheck for instance.<br>
<br>
Copyright:<br>
- Linaro (changes in version 2 of revision D55940)<br>
- GraphCore (changes in later versions and introduced when creating<br>
D56549)<br></blockquote><div><br></div><div>That copyright notice wasn't there when we reviewed. Likewise for D55940.</div><div><br></div><div>Was that an accident, or is this something people need to do now sometimes? I don't know whether it has any legal significance.<br></div><div><br></div><div>Thanks.<br></div><div><br></div><div>Joel<br></div><div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Reviewers: jdenny<br>
<br>
Subscribers: llvm-commits, probinson, kristina, hiraditya,<br>
JonChesterfield<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D56549" rel="noreferrer" target="_blank">https://reviews.llvm.org/D56549</a><br>
<br>
Modified:<br>
llvm/trunk/include/llvm/Support/CommandLine.h<br>
llvm/trunk/lib/Support/CommandLine.cpp<br>
llvm/trunk/unittests/Support/CommandLineTest.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/Support/CommandLine.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/CommandLine.h?rev=351038&r1=351037&r2=351038&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/CommandLine.h?rev=351038&r1=351037&r2=351038&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/Support/CommandLine.h (original)<br>
+++ llvm/trunk/include/llvm/Support/CommandLine.h Mon Jan 14 01:28:53 2019<br>
@@ -156,6 +156,9 @@ enum OptionHidden { // Control whether<br>
// enabled, and used, the value for the flag comes from the suffix of the<br>
// argument.<br>
//<br>
+// AlwaysPrefix - Only allow the behavior enabled by the Prefix flag and reject<br>
+// the Option=Value form.<br>
+//<br>
// Grouping - With this option enabled, multiple letter options are allowed to<br>
// bunch together with only a single hyphen for the whole group. This allows<br>
// emulation of the behavior that ls uses for example: ls -la === ls -l -a<br>
@@ -165,7 +168,8 @@ enum FormattingFlags {<br>
NormalFormatting = 0x00, // Nothing special<br>
Positional = 0x01, // Is a positional argument, no '-' required<br>
Prefix = 0x02, // Can this option directly prefix its value?<br>
- Grouping = 0x03 // Can this option group with other options?<br>
+ AlwaysPrefix = 0x03, // Can this option only directly prefix its value?<br>
+ Grouping = 0x04 // Can this option group with other options?<br>
};<br>
<br>
enum MiscFlags { // Miscellaneous flags to adjust argument<br>
@@ -265,7 +269,7 @@ class Option {<br>
// detail representing the non-value<br>
unsigned Value : 2;<br>
unsigned HiddenFlag : 2; // enum OptionHidden<br>
- unsigned Formatting : 2; // enum FormattingFlags<br>
+ unsigned Formatting : 3; // enum FormattingFlags<br>
unsigned Misc : 3;<br>
unsigned Position = 0; // Position of last occurrence of the option<br>
unsigned AdditionalVals = 0; // Greater than 0 for multi-valued option.<br>
<br>
Modified: llvm/trunk/lib/Support/CommandLine.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/CommandLine.cpp?rev=351038&r1=351037&r2=351038&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/CommandLine.cpp?rev=351038&r1=351037&r2=351038&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Support/CommandLine.cpp (original)<br>
+++ llvm/trunk/lib/Support/CommandLine.cpp Mon Jan 14 01:28:53 2019<br>
@@ -426,12 +426,17 @@ Option *CommandLineParser::LookupOption(<br>
return I != Sub.OptionsMap.end() ? I->second : nullptr;<br>
}<br>
<br>
- // If the argument before the = is a valid option name, we match. If not,<br>
- // return Arg unmolested.<br>
+ // If the argument before the = is a valid option name and the option allows<br>
+ // non-prefix form (ie is not AlwaysPrefix), we match. If not, signal match<br>
+ // failure by returning nullptr.<br>
auto I = Sub.OptionsMap.find(Arg.substr(0, EqualPos));<br>
if (I == Sub.OptionsMap.end())<br>
return nullptr;<br>
<br>
+ auto O = I->second;<br>
+ if (O->getFormattingFlag() == cl::AlwaysPrefix)<br>
+ return nullptr;<br>
+<br>
Value = Arg.substr(EqualPos + 1);<br>
Arg = Arg.substr(0, EqualPos);<br>
return I->second;<br>
@@ -539,7 +544,9 @@ static inline bool ProvideOption(Option<br>
switch (Handler->getValueExpectedFlag()) {<br>
case ValueRequired:<br>
if (!Value.data()) { // No value specified?<br>
- if (i + 1 >= argc)<br>
+ // If no other argument or the option only supports prefix form, we<br>
+ // cannot look at the next argument.<br>
+ if (i + 1 >= argc || Handler->getFormattingFlag() == cl::AlwaysPrefix)<br>
return Handler->error("requires a value!");<br>
// Steal the next argument, like for '-o filename'<br>
assert(argv && "null check");<br>
@@ -597,7 +604,8 @@ static inline bool isGrouping(const Opti<br>
return O->getFormattingFlag() == cl::Grouping;<br>
}<br>
static inline bool isPrefixedOrGrouping(const Option *O) {<br>
- return isGrouping(O) || O->getFormattingFlag() == cl::Prefix;<br>
+ return isGrouping(O) || O->getFormattingFlag() == cl::Prefix ||<br>
+ O->getFormattingFlag() == cl::AlwaysPrefix;<br>
}<br>
<br>
// getOptionPred - Check to see if there are any options that satisfy the<br>
@@ -647,7 +655,8 @@ HandlePrefixedOrGroupedOption(StringRef<br>
// If the option is a prefixed option, then the value is simply the<br>
// rest of the name... so fall through to later processing, by<br>
// setting up the argument name flags and value fields.<br>
- if (PGOpt->getFormattingFlag() == cl::Prefix) {<br>
+ if (PGOpt->getFormattingFlag() == cl::Prefix ||<br>
+ PGOpt->getFormattingFlag() == cl::AlwaysPrefix) {<br>
Value = Arg.substr(Length);<br>
Arg = Arg.substr(0, Length);<br>
assert(OptionsMap.count(Arg) && OptionsMap.find(Arg)->second == PGOpt);<br>
<br>
Modified: llvm/trunk/unittests/Support/CommandLineTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/CommandLineTest.cpp?rev=351038&r1=351037&r2=351038&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/CommandLineTest.cpp?rev=351038&r1=351037&r2=351038&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/unittests/Support/CommandLineTest.cpp (original)<br>
+++ llvm/trunk/unittests/Support/CommandLineTest.cpp Mon Jan 14 01:28:53 2019<br>
@@ -840,4 +840,78 @@ TEST(CommandLineTest, GetCommandLineArgu<br>
}<br>
#endif<br>
<br>
-} // anonymous namespace<br>
+TEST(CommandLineTest, PrefixOptions) {<br>
+ cl::ResetCommandLineParser();<br>
+<br>
+ StackOption<std::string, cl::list<std::string>> IncludeDirs(<br>
+ "I", cl::Prefix, cl::desc("Declare an include directory"));<br>
+<br>
+ // Test non-prefixed variant works with cl::Prefix options.<br>
+ EXPECT_TRUE(IncludeDirs.empty());<br>
+ const char *args[] = {"prog", "-I=/usr/include"};<br>
+ EXPECT_TRUE(<br>
+ cl::ParseCommandLineOptions(2, args, StringRef(), &llvm::nulls()));<br>
+ EXPECT_TRUE(IncludeDirs.size() == 1);<br>
+ EXPECT_TRUE(IncludeDirs.front().compare("/usr/include") == 0);<br>
+<br>
+ IncludeDirs.erase(IncludeDirs.begin());<br>
+ cl::ResetAllOptionOccurrences();<br>
+<br>
+ // Test non-prefixed variant works with cl::Prefix options when value is<br>
+ // passed in following argument.<br>
+ EXPECT_TRUE(IncludeDirs.empty());<br>
+ const char *args2[] = {"prog", "-I", "/usr/include"};<br>
+ EXPECT_TRUE(<br>
+ cl::ParseCommandLineOptions(3, args2, StringRef(), &llvm::nulls()));<br>
+ EXPECT_TRUE(IncludeDirs.size() == 1);<br>
+ EXPECT_TRUE(IncludeDirs.front().compare("/usr/include") == 0);<br>
+<br>
+ IncludeDirs.erase(IncludeDirs.begin());<br>
+ cl::ResetAllOptionOccurrences();<br>
+<br>
+ // Test prefixed variant works with cl::Prefix options.<br>
+ EXPECT_TRUE(IncludeDirs.empty());<br>
+ const char *args3[] = {"prog", "-I/usr/include"};<br>
+ EXPECT_TRUE(<br>
+ cl::ParseCommandLineOptions(2, args3, StringRef(), &llvm::nulls()));<br>
+ EXPECT_TRUE(IncludeDirs.size() == 1);<br>
+ EXPECT_TRUE(IncludeDirs.front().compare("/usr/include") == 0);<br>
+<br>
+ StackOption<std::string, cl::list<std::string>> MacroDefs(<br>
+ "D", cl::AlwaysPrefix, cl::desc("Define a macro"),<br>
+ cl::value_desc("MACRO[=VALUE]"));<br>
+<br>
+ cl::ResetAllOptionOccurrences();<br>
+<br>
+ // Test non-prefixed variant does not work with cl::AlwaysPrefix options:<br>
+ // equal sign is part of the value.<br>
+ EXPECT_TRUE(MacroDefs.empty());<br>
+ const char *args4[] = {"prog", "-D=HAVE_FOO"};<br>
+ EXPECT_TRUE(<br>
+ cl::ParseCommandLineOptions(2, args4, StringRef(), &llvm::nulls()));<br>
+ EXPECT_TRUE(MacroDefs.size() == 1);<br>
+ EXPECT_TRUE(MacroDefs.front().compare("=HAVE_FOO") == 0);<br>
+<br>
+ MacroDefs.erase(MacroDefs.begin());<br>
+ cl::ResetAllOptionOccurrences();<br>
+<br>
+ // Test non-prefixed variant does not allow value to be passed in following<br>
+ // argument with cl::AlwaysPrefix options.<br>
+ EXPECT_TRUE(MacroDefs.empty());<br>
+ const char *args5[] = {"prog", "-D", "HAVE_FOO"};<br>
+ EXPECT_FALSE(<br>
+ cl::ParseCommandLineOptions(3, args5, StringRef(), &llvm::nulls()));<br>
+ EXPECT_TRUE(MacroDefs.empty());<br>
+<br>
+ cl::ResetAllOptionOccurrences();<br>
+<br>
+ // Test prefixed variant works with cl::AlwaysPrefix options.<br>
+ EXPECT_TRUE(MacroDefs.empty());<br>
+ const char *args6[] = {"prog", "-DHAVE_FOO"};<br>
+ EXPECT_TRUE(<br>
+ cl::ParseCommandLineOptions(2, args6, StringRef(), &llvm::nulls()));<br>
+ EXPECT_TRUE(MacroDefs.size() == 1);<br>
+ EXPECT_TRUE(MacroDefs.front().compare("HAVE_FOO") == 0);<br>
+}<br>
+<br>
+} // anonymous namespace<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div></div>