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

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added inline comments.


================
Comment at: llvm/include/llvm/Option/ArgList.h:414
   }
+  void replaceArgString(unsigned Index, const Twine &S) {
+    ArgStrings[Index] = MakeArgString(S);
----------------
Add an empty line before the method.


================
Comment at: llvm/include/llvm/Option/OptTable.h:62
   bool IgnoreCase;
+  bool GroupedShortOptions = false;
 
----------------
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.


================
Comment at: llvm/lib/Option/OptTable.cpp:358
+      return A;
+    if (GroupedShortOptions && ArgSize == 2 &&
+        Opt.getKind() == Option::FlagClass)
----------------
The magic value `2` needs an explanatory comment.


================
Comment at: llvm/lib/Option/OptTable.cpp:358
+      return A;
+    if (GroupedShortOptions && ArgSize == 2 &&
+        Opt.getKind() == Option::FlagClass)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83639