[PATCH] D83639: [OptTable] Support grouped short options

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Option/OptTable.h:198-199
 
+  /// Parse a single argument; return the new argument and update Index. If
+  /// GroupedShortOptions is true, -a matches "-abc" and the argument in Args
+  /// will be updated to "-bc".
----------------
"return" -> "\return"
"Index" -> "\p Index"
"Args" -> "\p Args".

(i.e. use the doxygen style used elsewhere in this file)


================
Comment at: llvm/include/llvm/Option/Option.h:208-218
   /// accept - Potentially accept the current argument, returning a
   /// new Arg instance, or 0 if the option does not accept this
   /// argument (or the argument is missing values).
   ///
   /// If the option accepts the current argument, accept() sets
   /// Index to the position where argument parsing should resume
   /// (even if the argument is missing values).
----------------
This comment probably wants updating given the API change.


================
Comment at: llvm/lib/Option/OptTable.cpp:333
 
+Arg *OptTable::parseOneArg(InputArgList &Args, unsigned &Index) const {
+  // Anything that doesn't start with PrefixesUnion is an input, as is '-'
----------------
I've not looked too much in depth yet at this, but this function seems very similar to the one immediately below? Can the two somehow be combined?


================
Comment at: llvm/lib/Option/OptTable.cpp:350-351
+  for (; Start != End; ++Start) {
+    unsigned ArgSize;
+    if (!(ArgSize = matchOption(Start, Str, IgnoreCase)))
+      continue;
----------------
I feel like this would be more clearly written as:
```
    unsigned ArgSize = matchOption(Start, Str, IgnoreCase);
    if (!ArgSize)
```


================
Comment at: llvm/unittests/Option/OptionParsingTest.cpp:359-363
+  // Short options followed by a long option. We probably should disallow this.
+  const char *Args3[] = {"-AIblorp"};
+  InputArgList AL3 = T.ParseArgs(Args3, MAI, MAC);
+  EXPECT_TRUE(AL3.hasArg(OPT_A));
+  EXPECT_TRUE(AL3.hasArg(OPT_Blorp));
----------------
If we probably should disallow this, why don't we?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83639