[PATCH] D58499: [CommandLine] Do not crash if an option has both ValueRequired and Grouping.

Mon Feb 25 02:09:54 PST 2019

jhenderson accepted this revision.
jhenderson added a subscriber: rupprecht.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM.

In D58499#1407211 <https://reviews.llvm.org/D58499#1407211>, @ikudrin wrote:

> In D58499#1405561 <https://reviews.llvm.org/D58499#1405561>, @jhenderson wrote:
>
> > What happens for ValueOptional? I want to make sure that it doesn't treat the other grouped letters after it as the option's value.
>
>
> As far as I can see, in case of grouped options, the value might be assigned only if it is a prefixed option. Did you mean a specific case? Could you provide it so that I understand you better?
>
> Anyway, if anything needs fixing with `ValueOptional`, it should be a separate patch, right?

Yes, you're right. If there's any issue with ValueOptional, it should be a separate patch.  I don't have a specific case in mind. I just want to make sure that the behaviour of something like "-abc" where -a, -b and -c are all options, and -b is ValueOptional makes sense (I'm not sure whether it should be equivalent to "-a -b=c" or "-a -b -c" though). Although the code change is purely to do with ValueRequired, the documentation change is not specific to ValueRequired. Perhaps you could do a follow-up patch with at least a test for this behaviour, if it just works?

CC'ing @rupprecht, who may be interested in this change too.

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

https://reviews.llvm.org/D58499