Page MenuHomePhabricator

[CommandLine] Add callbacks to Options
ClosedPublic

Authored by hintonda on Nov 22 2019, 2:53 PM.

Details

Summary

Add a new cl::callback attribute to Option.

This attribute specifies a callback function that is called when
an option is seen, and can be used to set other options, as in
option A implies option B. If the option is a cl::list, and
cl::CommaSeparated is also specified, the callback will fire
once for each value. This could be used to validate combinations
or selectively set other options.

Diff Detail

Event Timeline

hintonda created this revision.Nov 22 2019, 2:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2019, 2:53 PM
hintonda updated this revision to Diff 230729.Nov 22 2019, 2:56 PM
  • Fix test case order.
Harbormaster completed remote builds in B41396: Diff 230729.

This looks nice. I think it's missing an entry somewhere in the user doc (so that people can start using it!). It would be great that in the auto generated -help this would add something like This option implies {name of the implied option}.

llvm/include/llvm/Support/CommandLine.h
292

Why limiting to a single ImpliedOption? It could be a list of implied options?

1394

Why limiting that top boolean? You could pass a callback that does extra processing based on the set value?

hintonda marked 2 inline comments as done.Nov 23 2019, 11:41 AM

This looks nice. I think it's missing an entry somewhere in the user doc (so that people can start using it!).

Thanks and sorry about not providing docs yet. Was waiting on initial feedback, but I'll add them as soon
as I figure out how to implement your suggestions.

The motivation for this change it to clean-up DebugOnlyOpt, which uses a custom datatype instead
of cl::list so it can also enable the Debug option. I need that for another patch I have in the works,
which sort-of explains the limited nature of my initial proposal. However, I like your suggestions, and
will try to implement them.

It would be great that in the auto generated -help this would add something like This option implies {name of the implied option}.

That's a good idea, and I'll work on adding it.

llvm/include/llvm/Support/CommandLine.h
292

I only needed one for my particular use case, but I believe you are correct that there's no reason to limit it to
a single implied option. Not sure about existing llvm tools, but there are plenty of examples of unix tools that
have this behavior.

1394

That's a great idea, and if we had a default method, we could support implied options via the same mechanism.
Let me see what I can come up with.

hintonda updated this revision to Diff 230794.Nov 24 2019, 8:07 AM
  • Add support for callbacks for Options, and remove cl::implied, since they are now OBE.
hintonda retitled this revision from [CommandLine] Add cl::implies option attribute to [CommandLine] Add callbacks to Options.Nov 24 2019, 8:12 AM
hintonda edited the summary of this revision. (Show Details)
hintonda marked an inline comment as done.Nov 24 2019, 8:16 AM
hintonda added inline comments.
llvm/include/llvm/Support/CommandLine.h
1394

@serge-sans-paille, I took your advice and added callbacks, which eliminated the need for cl::implied, so I removed it. Thanks again...

hintonda updated this revision to Diff 230795.Nov 24 2019, 8:18 AM
  • Fixed typos.

I'd just like to acknowledge a few issues with this patch, and request feedback:

  • the current version of Callback returns void, so it can't really be used to validate -- other than via assert/abort. If it returned a bool, it could be used to validate a specific value during parsing, but that doesn't apply to assignment, i.e., either directly or via nested callbacks -- that would require an assert/abort.
  • the Callback won't be invoked if the cl::location attribute is used and the location is mutated directly.

Given these limitations, I wonder if adding a callback to cl::ParseCommandLineOptions, and doing the validation there, might not be a better solution.

@hintonda I really prefer your version now, except the extra casting stuff that you can probably avoid (?)
Concerning your high level concerns, I don't have enough understanding of the big picture to help, sorry ^^!

llvm/include/llvm/Support/CommandLine.h
1384

A reinterpret_cast is not needed here

1444

Would this work if the function took a void const* instead? And again, the reinterpret_cast here is not needed.

hintonda updated this revision to Diff 230898.Nov 25 2019, 7:10 AM
  • Remove unnecessary casts.
beanz added a comment.Dec 2 2019, 2:42 PM

I like the idea behind this. I provided more detailed feedback to Don off-list, but I really don't like the const void* parameter to the callback functions. I think we can make that match the parser data type, and have compile-time errors for type mismatches. That would be vastly preferable to casting void pointers.

hintonda updated this revision to Diff 231805.EditedDec 2 2019, 6:09 PM
  • Remove void* and templatize callback struct.
MaskRay added inline comments.Dec 3 2019, 10:59 AM
llvm/docs/CommandLine.rst
1003
`cl::list`_
1004
`cl::CommaSeparated`_
beanz added inline comments.Dec 3 2019, 11:05 AM
llvm/include/llvm/Support/CommandLine.h
1455

Does this need to be a pointer or can it be a reference? Const reference passing seems preferable to const pointers to me.

hintonda updated this revision to Diff 231990.Dec 3 2019, 2:15 PM
  • Address comments.
serge-sans-paille added a comment.EditedDec 4 2019, 12:06 AM

LGTM, leaving it to @MaskRay or @beanz to accept it.

MaskRay added inline comments.Dec 4 2019, 9:53 AM
llvm/include/llvm/Support/CommandLine.h
480

Is the constructor needed? Can the call site below (cb<Ty>(CB);) just use aggregate initialization?

hintonda updated this revision to Diff 232253.Dec 4 2019, 7:26 PM
  • Extract arg type from callback so caller won't need to provide it.
hintonda updated this revision to Diff 232258.Dec 4 2019, 8:49 PM
  • Add static_asserts for arg_type
hintonda marked an inline comment as done.Dec 4 2019, 10:43 PM
hintonda added inline comments.
llvm/include/llvm/Support/CommandLine.h
480

I'm not sure I understand your question, but I've added function_traits to static_asserts to make it easier for callers to use and do the right thing. Please let me know if that addresses your concerns.

hintonda updated this revision to Diff 232353.Dec 5 2019, 8:08 AM
  • Rename traits and group asserts for readability. Add example to help.
beanz accepted this revision.Dec 5 2019, 2:41 PM

LGTM.

This revision is now accepted and ready to land.Dec 5 2019, 2:41 PM
MaskRay added inline comments.Dec 5 2019, 6:12 PM
llvm/include/llvm/Support/CommandLine.h
474

Delete callback -

hintonda updated this revision to Diff 232606.Dec 6 2019, 10:31 AM
  • Address comments
This revision was automatically updated to reflect the committed changes.