This is an archive of the discontinued LLVM Phabricator instance.

[CommandLine] Do not crash if an option has both ValueRequired and Grouping.
ClosedPublic

Authored by ikudrin on Feb 21 2019, 3:33 AM.

Details

Summary

If an option, which requires a value, has a cl::Grouping formatting flag,
it works well as far as it is used at the end of a group (or not in a group).
However, if the option appears accidentally in the middle of a group,
the program just crashes. I believe it is better to show some useful
error message in that case.

Diff Detail

Event Timeline

ikudrin created this revision.Feb 21 2019, 3:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2019, 3:33 AM
Herald added a subscriber: kristina. · View Herald Transcript

According to the documentation (https://llvm.org/docs/CommandLine.html#controlling-other-formatting-options) this is illegal even for the last value:

Note that cl::Grouping options cannot have values.

Does it just work? If so, great, but please update the docs.

Regardless, we certainly shouldn't be asserting if we can get into that state, so this is a good change.

Your description mentioned ValueRequired. 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.

unittests/Support/CommandLineTest.cpp
1146

The first option isn't really in the middle, after all, so this comment should be adjusted to say "elswhere in the group" or "not at the end of a group" or similar.

ikudrin updated this revision to Diff 187944.Feb 22 2019, 8:58 AM
  • Updated the documentation (feel free to correct my wording).
  • Amended a comment in the test.

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?

jhenderson accepted this revision.Feb 25 2019, 2:06 AM
jhenderson added a subscriber: rupprecht.

LGTM.

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.

This revision is now accepted and ready to land.Feb 25 2019, 2:06 AM

LGTM.

Thanks!

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).

If I understand it right, a ValueOptional option can accept its value only if it is explicitly used in the -opt=value form. Moreover, as boolean options are of a ValueOptional kind implicitly, your sample is something very basic for checking how Grouping works at all. "-abc" should definitely be equivalent to "-a -b -c". It is a bit strange that there are no tests for that. Maybe the test suite is worth to be extended.

Although the code change is purely to do with ValueRequired, the documentation change is not specific to ValueRequired.

You are right. We either need better wording in the documentation or improve the code so that the "=val" form is accepted with a group. I would prefer changing the documentation because no one needed that new behavior yet.

Perhaps you could do a follow-up patch with at least a test for this behaviour, if it just works?

Let's create a separate patch with additional tests for the existing behavior.

ormris added a subscriber: ormris.Feb 25 2019, 11:24 AM
This revision was automatically updated to reflect the committed changes.