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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/include/llvm/Option/OptTable.h:62
   bool IgnoreCase;
+  bool GroupedShortOptions = false;
 
----------------
ikudrin wrote:
> Wouldn't it be better if the grouping is a property of options themselves rather then a flag in the parser? That would allow better tuning of options.
Grouped short options is all or nothing. I think it is hardly the case that a utility supports some short options being grouped. This is a compromised design in CommandLine AFAICT.


================
Comment at: llvm/lib/Option/OptTable.cpp:358
+      return A;
+    if (GroupedShortOptions && ArgSize == 2 &&
+        Opt.getKind() == Option::FlagClass)
----------------
ikudrin wrote:
> ikudrin wrote:
> > The magic value `2` needs an explanatory comment.
> The code can't get here if `GroupedShortOptions` is `false`, no? Though, I'd prefer the two parsing methods to be merged.
Removed `GroupedShortOptions`. I have explained that some call sites require refactoring (due to const reference) if we want to merge parseOneOption & ParseOneOption.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83639