[CommandLine] Add callbacks to Options

[hintonda]

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.

[hintonda]

• Fix test case order.

[serge-sans-paille]

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

[serge-sans-paille]

llvm/include/llvm/Support/CommandLine.h

289

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

1369

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

[hintonda]

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

llvm/include/llvm/Support/CommandLine.h

289

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.

1369

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]

• Add support for callbacks for Options, and remove cl::implied, since they are now OBE.

[hintonda]

llvm/include/llvm/Support/CommandLine.h

1369

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

[hintonda]

• Fixed typos.

[hintonda]

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.

[serge-sans-paille]

@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

1359

A reinterpret_cast is not needed here

1419

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

[hintonda]

• Remove unnecessary casts.

[beanz]

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]

• Remove void* and templatize callback struct.

[MaskRay]

llvm/docs/CommandLine.rst

1003

`cl::list`_

1004

`cl::CommaSeparated`_

[beanz]

llvm/include/llvm/Support/CommandLine.h

1430

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

[hintonda]

• Address comments.

[serge-sans-paille]

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

[MaskRay]

llvm/include/llvm/Support/CommandLine.h

477

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

[hintonda]

• Extract arg type from callback so caller won't need to provide it.

[hintonda]

• Add static_asserts for arg_type

[hintonda]

llvm/include/llvm/Support/CommandLine.h

477

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]

• Rename traits and group asserts for readability. Add example to help.

[beanz]

LGTM.

[MaskRay]

llvm/include/llvm/Support/CommandLine.h

471

Delete callback -

[hintonda]

• Address comments

llvm/docs/CommandLine.rst

* The **cl::multi_val** attribute specifies that this option takes has multiple
   of the usual modifiers on multi-valued options (besides
   ``cl::ValueDisallowed``, obviously).
 
 .. _cl::cat:
 
 * The **cl::cat** attribute specifies the option category that the option
   belongs to. The category should be a `cl::OptionCategory`_ object.
 
+.. _cl::callback:
+
+* The **cl::callback** 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.
+
 Option Modifiers
 ----------------
 
 Option modifiers are the flags and expressions that you pass into the
 constructors for `cl::opt`_ and `cl::list`_.  These modifiers give you the
 ability to tweak how options are parsed and how ``-help`` output is generated to
 fit your application well.
 

llvm/docs/ReleaseNotes.rst

* As per :ref:`LLVM Language Reference Manual <i_getelementptr>`,
   will now catch such cases.
 
 
 * Windows Control Flow Guard: the ``-cfguard`` option now emits CFG checks on
   indirect function calls. The previous behavior is still available with the 
   ``-cfguard-nochecks`` option. Note that this feature should always be used 
   with optimizations enabled.
 
+* ``Callbacks`` have been added to ``CommandLine Options``.  These can
+  be used to validate of selectively enable other options.
+
 Changes to the LLVM IR
 ----------------------
 
 * Unnamed function arguments now get printed with their automatically
   generated name (e.g. "i32 %0") in definitions. This may require front-ends
   to update their tests; if so there is a script utils/add_argument_names.py
   that correctly converted 80-90% of Clang tests. Some manual work will almost
   certainly still be needed.

llvm/include/llvm/Support/CommandLine.h

 
 public:
   StringRef ArgStr;   // The argument string itself (ex: "help", "o")
   StringRef HelpStr;  // The descriptive text message for -help
   StringRef ValueStr; // String describing what the value of this option is
   SmallVector<OptionCategory *, 1>
       Categories;                    // The Categories this option belongs to
   SmallPtrSet<SubCommand *, 1> Subs; // The subcommands this option belongs to.
 
   inline enum NumOccurrencesFlag getNumOccurrencesFlag() const {
     return (enum NumOccurrencesFlag)Occurrences;
   }
 
   inline enum ValueExpected getValueExpectedFlag() const {
     return Value ? ((enum ValueExpected)Value) : getValueExpectedFlagDefault();
   }
 

 struct sub {
   SubCommand &Sub;
 
   sub(SubCommand &S) : Sub(S) {}
 
   template <class Opt> void apply(Opt &O) const { O.addSubCommand(Sub); }
 };
 
+// callback - Specify a callback function to be called when an option is seen.
+// Can be used to set other options automatically.
+template <typename Ty> struct cb {
+  std::function<void(const Ty *)> CB;
+
+  cb(std::function<void(const Ty *)> CB) : CB(CB) {}
+
+  template <typename Opt> void apply(Opt &O) const { O.setCallback(CB); }
+};
+
+template <typename Ty, typename FTy = std::function<void(const Ty *)>>
+cb<Ty> callback(FTy CB) {
+  return cb<Ty>(CB);
+}
+
 //===----------------------------------------------------------------------===//
 // OptionValue class
 
 // Support value comparison outside the template.
 struct GenericOptionValue {
   virtual bool compare(const GenericOptionValue &V) const = 0;
 
 protected:
class opt : public Option,
   bool handleOccurrence(unsigned pos, StringRef ArgName,
                         StringRef Arg) override {
     typename ParserClass::parser_data_type Val =
         typename ParserClass::parser_data_type();
     if (Parser.parse(*this, ArgName, Arg, Val))
       return true; // Parse error!
     this->setValue(Val);
     this->setPosition(pos);
+    Callback(&Val);
     return false;
   }
 
   enum ValueExpected getValueExpectedFlagDefault() const override {
     return Parser.getValueExpectedFlagDefault();
   }
 
   void getExtraOptionNames(SmallVectorImpl<StringRef> &OptionNames) override {
     return Parser.getExtraOptionNames(OptionNames);
   }
 
   // Forward printing stuff to the parser...
   size_t getOptionWidth() const override {
     return Parser.getOptionWidth(*this);
   }
 
   void printOptionInfo(size_t GlobalWidth) const override {
     Parser.printOptionInfo(*this, GlobalWidth);
public:
 
   // setInitialValue - Used by the cl::init modifier...
   void setInitialValue(const DataType &V) { this->setValue(V, true); }
 
   ParserClass &getParser() { return Parser; }
 
   template <class T> DataType &operator=(const T &Val) {
     this->setValue(Val);
+    Callback(&Val);
     return this->getValue();
   }
 
   template <class... Mods>
   explicit opt(const Mods &... Ms)
       : Option(Optional, NotHidden), Parser(*this) {
     apply(this, Ms...);
     done();
   }
+
+  void setCallback(
+      std::function<void(const typename ParserClass::parser_data_type *)> CB) {
+    Callback = CB;
+  }
+
+  std::function<void(const typename ParserClass::parser_data_type *)> Callback =
+      [](const typename ParserClass::parser_data_type *) {};
 };
 
 extern template class opt<unsigned>;
 extern template class opt<int>;
 extern template class opt<std::string>;
 extern template class opt<char>;
 extern template class opt<bool>;
 
bool handleOccurrence(unsigned pos, StringRef ArgName,
                         StringRef Arg) override {
     typename ParserClass::parser_data_type Val =
         typename ParserClass::parser_data_type();
     if (Parser.parse(*this, ArgName, Arg, Val))
       return true; // Parse Error!
     list_storage<DataType, StorageClass>::addValue(Val);
     setPosition(pos);
     Positions.push_back(pos);
+    Callback(&Val);
     return false;
   }
 
   // Forward printing stuff to the parser...
   size_t getOptionWidth() const override {
     return Parser.getOptionWidth(*this);
   }
 
public:
   void setNumAdditionalVals(unsigned n) { Option::setNumAdditionalVals(n); }
 
   template <class... Mods>
   explicit list(const Mods &... Ms)
       : Option(ZeroOrMore, NotHidden), Parser(*this) {
     apply(this, Ms...);
     done();
   }
+
+  void setCallback(
+      std::function<void(const typename ParserClass::parser_data_type *)> CB) {
+    Callback = CB;
+  }
+
+  std::function<void(const typename ParserClass::parser_data_type *)> Callback =
+      [](const typename ParserClass::parser_data_type *) {};
 };
 
 // multi_val - Modifier to set the number of additional values.
 struct multi_val {
   unsigned AdditionalVals;
   explicit multi_val(unsigned N) : AdditionalVals(N) {}
 
   template <typename D, typename S, typename P>
bool handleOccurrence(unsigned pos, StringRef ArgName,
                         StringRef Arg) override {
     typename ParserClass::parser_data_type Val =
         typename ParserClass::parser_data_type();
     if (Parser.parse(*this, ArgName, Arg, Val))
       return true; // Parse Error!
     this->addValue(Val);
     setPosition(pos);
     Positions.push_back(pos);
+    Callback(&Val);
     return false;
   }
 
   // Forward printing stuff to the parser...
   size_t getOptionWidth() const override {
     return Parser.getOptionWidth(*this);
   }
 

llvm/unittests/Support/CommandLineTest.cpp

 class StackOption : public Base {
 public:
   template <class... Ts>
   explicit StackOption(Ts &&... Ms) : Base(std::forward<Ts>(Ms)...) {}
 
   ~StackOption() override { this->removeArgument(); }
 
   template <class DT> StackOption<T> &operator=(const DT &V) {
-    this->setValue(V);
+    Base::operator=(V);
     return *this;
   }
 };
 
 class StackSubCommand : public cl::SubCommand {
 public:
   StackSubCommand(StringRef Name,
                   StringRef Description = StringRef())
TEST(CommandLineTest, OptionErrorMessageSuggest) {
   EXPECT_FALSE(cl::ParseCommandLineOptions(2, args, StringRef(), &OS));
   OS.flush();
   EXPECT_FALSE(Errs.find("prog: Did you mean '--aluminium'?\n") ==
                std::string::npos);
   Errs.clear();
 
   cl::ResetAllOptionOccurrences();
 }
-}  // anonymous namespace
+
+TEST(CommandLineTest, Callback) {
+  cl::ResetCommandLineParser();
+
+  StackOption<bool> OptA("a", cl::desc("option a"));
+  StackOption<bool> OptB(
+      "b", cl::desc("option b -- This option turns on option a"),
+      cl::callback<bool>([&](const bool *) { OptA = true; }));
+  StackOption<bool> OptC(
+      "c", cl::desc("option c -- This option turns on options a and b"),
+      cl::callback<bool>([&](const bool *) { OptB = true; }));
+  StackOption<std::string, cl::list<std::string>> List(
+      "list",
+      cl::desc("option list -- This option turns on options a, b, and c when "
+               "'foo' is included in list"),
+      cl::CommaSeparated,
+      cl::callback<std::string>([&](const std::string *Str) {
+        if (*Str == "foo")
+          OptC = true;
+      }));
+
+  const char *args1[] = {"prog", "-a"};
+  EXPECT_TRUE(cl::ParseCommandLineOptions(2, args1));
+  EXPECT_TRUE(OptA);
+  EXPECT_FALSE(OptB);
+  EXPECT_FALSE(OptC);
+  EXPECT_TRUE(List.size() == 0);
+  cl::ResetAllOptionOccurrences();
+
+  const char *args2[] = {"prog", "-b"};
+  EXPECT_TRUE(cl::ParseCommandLineOptions(2, args2));
+  EXPECT_TRUE(OptA);
+  EXPECT_TRUE(OptB);
+  EXPECT_FALSE(OptC);
+  EXPECT_TRUE(List.size() == 0);
+  cl::ResetAllOptionOccurrences();
+
+  const char *args3[] = {"prog", "-c"};
+  EXPECT_TRUE(cl::ParseCommandLineOptions(2, args3));
+  EXPECT_TRUE(OptA);
+  EXPECT_TRUE(OptB);
+  EXPECT_TRUE(OptC);
+  EXPECT_TRUE(List.size() == 0);
+  cl::ResetAllOptionOccurrences();
+
+  const char *args4[] = {"prog", "--list=foo,bar"};
+  EXPECT_TRUE(cl::ParseCommandLineOptions(2, args4));
+  EXPECT_TRUE(OptA);
+  EXPECT_TRUE(OptB);
+  EXPECT_TRUE(OptC);
+  EXPECT_TRUE(List.size() == 2);
+  cl::ResetAllOptionOccurrences();
+
+  const char *args5[] = {"prog", "--list=bar"};
+  EXPECT_TRUE(cl::ParseCommandLineOptions(2, args5));
+  EXPECT_FALSE(OptA);
+  EXPECT_FALSE(OptB);
+  EXPECT_FALSE(OptC);
+  EXPECT_TRUE(List.size() == 1);
+
+  cl::ResetAllOptionOccurrences();
+}
+} // anonymous namespace