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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
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 '-'
----------------
jhenderson wrote:
> 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?
Answered below.


================
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)
----------------
grimar wrote:
> What happens when `GroupedShortOptions` is true and `FlagsToInclude`/`FlagsToExclude` are present?
There is no such use case.

FlagsToInclude/FlagsToExclude is only used by clang-cl. clang does not use grouped short options.


================
Comment at: llvm/lib/Option/OptTable.cpp:467
+                 ? parseOneArg(Args, Index)
+                 : ParseOneArg(Args, Index, FlagsToInclude, FlagsToExclude);
+    assert((Index > Prev || GroupedShortOptions) &&
----------------
grimar wrote:
> 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?
I have mentioned in the description that having two methods is not great but isn't a bad choice.

> ... The implementation can be simpler if ArgList is mutable.
(ParseOneArg is used by clang-cl (FlagsToInclude/FlagsToExclude) and lld COFF
(case-insensitive). Adding grouped short options can make the function even more
complex.)

ParseOneArg requires `const ArgList &Args` in some clang call sites. The interface has been made complex by clang-cl and lld COFF (case-insensitive parsing). Having a duplicate parseOneArg actually isolates the complexity without adding too many lines.

In the future, we may consider dropping `-long-option` support from this overload. This may allow us to optimize/simplify the function. Combining it with ParseShortOptions may add a large switch inside a for loop, which may not be desired.


================
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));
----------------
jhenderson wrote:
> If we probably should disallow this, why don't we?
This syntax only make sense when there is one dash. If we drop `-long-option` support, this will be naturally disallowed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83639