[PATCH] D58711: [CommandLine] Allow grouping of options which can have values.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a subscriber: ormris.
jhenderson added inline comments.


================
Comment at: docs/CommandLine.rst:1201
+  4. while (!Input.empty()) {
+    string MaybeValue = OrigInput.substr(Input.length())
+    if (getOption(Input).isPrefix())
----------------
The indentation here looks off compared to the old style?


================
Comment at: lib/Support/CommandLine.cpp:682-683
 
-    // Because the value for the option is not required, we don't need to pass
-    // argc/argv in.
+    // Because the value for the option is not required,
+    // we don't need to pass argc/argv in.
     int Dummy = 0;
----------------
Why has this changed?


================
Comment at: lib/Support/CommandLine.cpp:692
 
-  // Return the last option with Arg cut down to just the last one.
-  return PGOpt;
+  // We could not find a grouping option in the remaining of Arg.
+  return nullptr;
----------------
remaining -> remainder


================
Comment at: unittests/Support/CommandLineTest.cpp:1176
+
+  // Should assign an empty value if such an option is used elsewhere
+  // in the group.
----------------
Maybe better to be explicit here instead of using "such", i.e. "if a ValueOptional option..."


================
Comment at: unittests/Support/CommandLineTest.cpp:1225
+
+  // The all three previous cases should work the same way
+  // if a cl::Prefix + cl::Grouping option is used at the end of a group.
----------------
The all -> All

The wrapping here looks a bit weird. I think "if a" should be on this line, not the next.


================
Comment at: unittests/Support/CommandLineTest.cpp:1270-1271
+
+  // The all three previous cases should work the same way
+  // if a cl::AlwaysPrefix + cl::Grouping option is used at the end of a group.
+  const char *args10[] = {"prog", "-faval10"};
----------------
Same as above comment re. "The all" and "if a".


================
Comment at: unittests/Support/CommandLineTest.cpp:1289
+  OptA.clear();
+  cl::ResetAllOptionOccurrences();
 }
----------------
What about a test case for "-fab" where "a", "f" and "b" are all options, and "a" is a Prefix/AlwaysPrefix option? I think that would clearly show that "b" would be the value of "a".


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58711/new/

https://reviews.llvm.org/D58711