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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/Option/OptTable.cpp:465
     unsigned Prev = Index;
-    Arg *A = ParseOneArg(Args, Index, FlagsToInclude, FlagsToExclude);
-    assert(Index > Prev && "Parser failed to consume argument.");
+    Arg *A = GroupedShortOptions
+                 ? parseOneArg(Args, Index)
----------------
What happens when `GroupedShortOptions` is true and `FlagsToInclude`/`FlagsToExclude` are present?


================
Comment at: llvm/lib/Option/OptTable.cpp:467
+                 ? parseOneArg(Args, Index)
+                 : ParseOneArg(Args, Index, FlagsToInclude, FlagsToExclude);
+    assert((Index > Prev || GroupedShortOptions) &&
----------------
This looks strange. We should not have 2 versions of `parseOneArg`/`ParseOneArg` that have different naming styles (lower-case vs upper-case) of the same name.

Also, their signatures (given that `InputArgList` inherits from `ArgList`) are very close:

```
  Arg *ParseOneArg(const ArgList &Args, unsigned &Index,
                   unsigned FlagsToInclude = 0,
                   unsigned FlagsToExclude = 0) const;

  Arg *parseOneArg(InputArgList &Args, unsigned &Index) const;
```
  
And it is probably too easy to call one instead of another.
So it looks like either they should be combined, or, if it is not possible, probably the new `parseOneArg` should be renamed to something else?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83639