[PATCH] D58711: [CommandLine] Allow grouping options which can have values.

Thu Feb 28 03:13:16 PST 2019

jhenderson added a comment.

In D58711#1413416 <https://reviews.llvm.org/D58711#1413416>, @ikudrin wrote:

> Do you think it is worth adding a new modifier, say, `cl::PreserveEqSign`, to improve compatibility?

I'm not really sure. I don't know enough about the background of the CommandLine library and its aims, so I'm reluctant to suggest adding more modifiers for what might be corner behaviours that aren't used. Perhaps worth putting up a small RFC on llvm-dev?

================
Comment at: unittests/Support/CommandLineTest.cpp:1225
+
+  // The all three previous cases should work the same way
+  // if a cl::Prefix + cl::Grouping option is used at the end of a group.
----------------
ikudrin wrote:
> jhenderson wrote:
> > The all -> All
> > 
> > The wrapping here looks a bit weird. I think "if a" should be on this line, not the next.
> I wanted not to split `cl::Prefix + cl::Grouping`, as well as not to separate an article from them.
> So, I opted to rephrase the comment. How does it look now?
Yes, it's fine.

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

https://reviews.llvm.org/D58711