[PATCH] D70620: [CommandLine] Add cl::implies option attribute

[Don Hinton via Phabricator via llvm-commits]

hintonda marked 2 inline comments as done.
hintonda added a comment.

In D70620#1757692 <https://reviews.llvm.org/D70620#1757692>, @serge-sans-paille wrote:

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



================
Comment at: llvm/include/llvm/Support/CommandLine.h:289
   SmallPtrSet<SubCommand *, 1> Subs; // The subcommands this option belongs to.
+  Option *ImpliedOption = nullptr;
 
----------------
serge-sans-paille wrote:
> Why limiting to a single ImpliedOption? It could be a list of implied options?
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.


================
Comment at: llvm/include/llvm/Support/CommandLine.h:1364
+
+  // setImplied is only supported for enabling boolean options.
+  template <typename T, typename std::enable_if<std::is_same<T, bool>::value,
----------------
serge-sans-paille wrote:
> Why limiting that top boolean? You could pass a callback that does extra processing based on the set value?
> 
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70620