[Support] CommandLine API -- Allow creating custom parsers for fundamental types

[ckristo]

Summary

I lately stumbled upon the bug #25104 while I wanted to create a custom parser for a cl::opt<float> command line option. Since cl::parser<T> implementations were marked final in 3.7, it is no longer possible to derive from them as described in the CommandLine documentation. Deriving from cl::basic_parser<T> for fundamental types is not possible either, causing a compile error like

llvm/include/llvm/Support/CommandLine.h:1109:20: No viable conversion from 'const (anonymous namespace)::CustomParser' to 'const parser<T>'

This small patch allows to derive from cl::basic_parser<T> for fundamental types. Please note that this patch does not allow to build upon existing parsers again (as prev. possible by inheriting from cl::parser<T>), i.e. custom parser for numeric options must reimplement number parsing. This is not ideal, I know, but I do not have an idea how to make this easily possible again without removing final from cl::parser<T>. Any ideas that can be incorporated in this patch are welcome of course :-)

I kindly ask for review.

~~~~~~~~~~
Chris

[ckristo]

Accidentally uploaded the wrong diff, sorry. Now the correct diff is presented for review.

[ckristo]

include/llvm/Support/CommandLine.h

1054–1056

I'll remove these three lines in the next revision -- this slipped through code cleanup, sorry!

[serge-sans-paille]

+1 for this patch.

I have the feeling that the virtual qualifier on printOptionDiff could be avoided, as this method does not seem to be called through base class at any point ?

[ckristo]

Removed some lines that were commented out and slipped through initial code cleanup + recreated patch with correct svn diff command for more context.

[ckristo]

In D41674#967275, @serge-sans-paille wrote:

+1 for this patch.

Hi,

Thanks for reviewing my little patch :-)

In D41674#967275, @serge-sans-paille wrote:

I have the feeling that the virtual qualifier on printOptionDiff could be avoided, as this method does not seem to be called through base class at any point ?

I tried to avoid making printOptionDiff virtual, but I ended up in calling basic_parser's (dummy) method implementation instead of the concrete child class' implementation. Removing virtual might be possible when revisiting the PrintOptionDiff template methods (lines 1086ff) to always pass the parser type somehow (and making a cast to the concrete parser type before calling the parser's printOptionDiff method), but to be honest I did not want to touch the templates much because I do not really get what they're doing/needed for and which version is selected in which case. Furthermore, I thought making printOptionDiff virtual does not make much of a difference because basic_parser had virtual functions already. Anyhow, if you have more insight in the PrintOptionDiff template voodoo and see a method in making printOptionDiff non-virtual, I'd be happy if you could guide me here.

[serge-sans-paille]

@ckristo sorry for the delay, I just realized that you missed to update the cl documentation, which is the actual point of this patch :-) Otherwise LGTM

[chandlerc]

I have a feeling that the original bug was from a time where we generally thought this side of commandline infrastructure was going to become a more broadly used way of building commandline tools with rich commandline interfaces. However, in recent years the project has gravitated towards the infrastructure in the Option library which is used by both Clang and LLD.

So, I'm personally not inclined to add more complexity to the commandline parsing logic. Is there a particular need that motivated this? I understand you wanted a float option, but for what purpose? Generally, we've been successful using basic integer ratios for debugging and testing options.

[serge-sans-paille]

We (= the Quarkslab firm) have two use cases for extending the parsers:

• using floating point numbers for obfuscation rations (as in « obfuscate n % of the constants). sometimes the ratio is 0.001 or such
• using regular expression over function names. In that case it is also convenient to be able to type the option as a regexp and not only a string.

These options are used from regular passes. Is the Option library a good alternative in that case?

[chandlerc]

In D41674#1048966, @serge-sans-paille wrote:

We (= the Quarkslab firm) have two use cases for extending the parsers:

• using floating point numbers for obfuscation rations (as in « obfuscate n % of the constants). sometimes the ratio is 0.001 or such
• using regular expression over function names. In that case it is also convenient to be able to type the option as a regexp and not only a string.

Is this for debugging / tuning / development?

If so, seems reasonable to use a an integer / 1000 (or larger) for ratios.

Not sure why the regexp option wouldn't be best suited to a custom type anyways. Seems like you don't need custom parsers for fundamental types there.

These options are used from regular passes. Is the Option library a good alternative in that case?

If you expect these to be customized outside of debugging / development of the passes themselves, they should be designed as part of hte API of the pass for library consumers. Then, when constructing the pass you can parameterize it in whatever way you want using data parsed out of whatever commandline library you want?

[serge-sans-paille]

Interesting. Historically, we built an out-of-tree compiler, so llvm::cl was the natural choice to control pass behavior. Now that we work in-tree, using llvm::Option becomes a viable option.

Going that way, this patch should be repalced by a documentation patch that no longer advocates for subtyping basic types at all.

include/llvm/Support/CommandLine.h

 };
 
 // basic_parser - The real basic parser is just a template wrapper that provides
 // a typedef for the provided data type.
 //
 template <class DataType> class basic_parser : public basic_parser_impl {
 public:
   using parser_data_type = DataType;
+  
+  using Val = DataType;
+  using ValParam = typename std::conditional<std::is_scalar<DataType>::value,
+                                             Val, const Val&>::type;
   using OptVal = OptionValue<DataType>;
+  using OptValParam = typename std::conditional<std::is_scalar<DataType>::value,
+                                                OptVal, const OptVal&>::type;
 
   basic_parser(Option &O) : basic_parser_impl(O) {}
   
+  virtual void printOptionDiff(const Option &O, ValParam V, OptValParam Default,
+                               size_t GlobalWidth) const {
+    printOptionNoValue(O, GlobalWidth);
+  }
+
 protected:
   ~basic_parser() = default;
 };
 
 //--------------------------------------------------
 // parser<bool>
 //
 template <> class parser<bool> final : public basic_parser<bool> {
 public:
   parser(Option &O) : basic_parser(O) {}
 
   // parse - Return true on error.
   bool parse(Option &O, StringRef ArgName, StringRef Arg, bool &Val);
 
   void initialize() {}
 
   enum ValueExpected getValueExpectedFlagDefault() const {
     return ValueOptional;
   }
 
   // getValueName - Do not print =<value> at all.
   StringRef getValueName() const override { return StringRef(); }
 
   void printOptionDiff(const Option &O, bool V, OptVal Default,
-                       size_t GlobalWidth) const;
+                       size_t GlobalWidth) const override;
 
   // An out-of-line virtual method to provide a 'home' for this class.
   void anchor() override;
 };
 
 extern template class basic_parser<bool>;
 
 //--------------------------------------------------
public:
   enum ValueExpected getValueExpectedFlagDefault() const {
     return ValueOptional;
   }
 
   // getValueName - Do not print =<value> at all.
   StringRef getValueName() const override { return StringRef(); }
 
   void printOptionDiff(const Option &O, boolOrDefault V, OptVal Default,
-                       size_t GlobalWidth) const;
+                       size_t GlobalWidth) const override;
 
   // An out-of-line virtual method to provide a 'home' for this class.
   void anchor() override;
 };
 
 extern template class basic_parser<boolOrDefault>;
 
 //--------------------------------------------------
 // parser<int>
 //
 template <> class parser<int> final : public basic_parser<int> {
 public:
   parser(Option &O) : basic_parser(O) {}
 
   // parse - Return true on error.
   bool parse(Option &O, StringRef ArgName, StringRef Arg, int &Val);
 
   // getValueName - Overload in subclass to provide a better default value.
   StringRef getValueName() const override { return "int"; }
 
   void printOptionDiff(const Option &O, int V, OptVal Default,
-                       size_t GlobalWidth) const;
+                       size_t GlobalWidth) const override;
 
   // An out-of-line virtual method to provide a 'home' for this class.
   void anchor() override;
 };
 
 extern template class basic_parser<int>;
 
 //--------------------------------------------------
 // parser<unsigned>
 //
 template <> class parser<unsigned> final : public basic_parser<unsigned> {
 public:
   parser(Option &O) : basic_parser(O) {}
 
   // parse - Return true on error.
   bool parse(Option &O, StringRef ArgName, StringRef Arg, unsigned &Val);
 
   // getValueName - Overload in subclass to provide a better default value.
   StringRef getValueName() const override { return "uint"; }
 
   void printOptionDiff(const Option &O, unsigned V, OptVal Default,
-                       size_t GlobalWidth) const;
+                       size_t GlobalWidth) const override;
 
   // An out-of-line virtual method to provide a 'home' for this class.
   void anchor() override;
 };
 
 extern template class basic_parser<unsigned>;
 
 //--------------------------------------------------
 // parser<unsigned long long>
 //
 template <>
 class parser<unsigned long long> final
     : public basic_parser<unsigned long long> {
 public:
   parser(Option &O) : basic_parser(O) {}
 
   // parse - Return true on error.
   bool parse(Option &O, StringRef ArgName, StringRef Arg,
              unsigned long long &Val);
 
   // getValueName - Overload in subclass to provide a better default value.
   StringRef getValueName() const override { return "uint"; }
 
   void printOptionDiff(const Option &O, unsigned long long V, OptVal Default,
-                       size_t GlobalWidth) const;
+                       size_t GlobalWidth) const override;
 
   // An out-of-line virtual method to provide a 'home' for this class.
   void anchor() override;
 };
 
 extern template class basic_parser<unsigned long long>;
 
 //--------------------------------------------------
 // parser<double>
 //
 template <> class parser<double> final : public basic_parser<double> {
 public:
   parser(Option &O) : basic_parser(O) {}
 
   // parse - Return true on error.
   bool parse(Option &O, StringRef ArgName, StringRef Arg, double &Val);
 
   // getValueName - Overload in subclass to provide a better default value.
   StringRef getValueName() const override { return "number"; }
 
   void printOptionDiff(const Option &O, double V, OptVal Default,
-                       size_t GlobalWidth) const;
+                       size_t GlobalWidth) const override;
 
   // An out-of-line virtual method to provide a 'home' for this class.
   void anchor() override;
 };
 
 extern template class basic_parser<double>;
 
 //--------------------------------------------------
 // parser<float>
 //
 template <> class parser<float> final : public basic_parser<float> {
 public:
   parser(Option &O) : basic_parser(O) {}
 
   // parse - Return true on error.
   bool parse(Option &O, StringRef ArgName, StringRef Arg, float &Val);
 
   // getValueName - Overload in subclass to provide a better default value.
   StringRef getValueName() const override { return "number"; }
 
   void printOptionDiff(const Option &O, float V, OptVal Default,
-                       size_t GlobalWidth) const;
+                       size_t GlobalWidth) const override;
 
   // An out-of-line virtual method to provide a 'home' for this class.
   void anchor() override;
 };
 
 extern template class basic_parser<float>;
 
 //--------------------------------------------------
 // parser<std::string>
 //
 template <> class parser<std::string> final : public basic_parser<std::string> {
 public:
   parser(Option &O) : basic_parser(O) {}
 
   // parse - Return true on error.
   bool parse(Option &, StringRef, StringRef Arg, std::string &Value) {
     Value = Arg.str();
     return false;
   }
 
   // getValueName - Overload in subclass to provide a better default value.
   StringRef getValueName() const override { return "string"; }
 
-  void printOptionDiff(const Option &O, StringRef V, const OptVal &Default,
-                       size_t GlobalWidth) const;
+  void printOptionDiff(const Option &O, const std::string &V,
+                       const OptVal &Default,
+                       size_t GlobalWidth) const override;
 
   // An out-of-line virtual method to provide a 'home' for this class.
   void anchor() override;
 };
 
 extern template class basic_parser<std::string>;
 
 //--------------------------------------------------
 // parser<char>
 //
 template <> class parser<char> final : public basic_parser<char> {
 public:
   parser(Option &O) : basic_parser(O) {}
 
   // parse - Return true on error.
   bool parse(Option &, StringRef, StringRef Arg, char &Value) {
     Value = Arg[0];
     return false;
   }
 
   // getValueName - Overload in subclass to provide a better default value.
   StringRef getValueName() const override { return "char"; }
 
   void printOptionDiff(const Option &O, char V, OptVal Default,
-                       size_t GlobalWidth) const;
+                       size_t GlobalWidth) const override;
 
   // An out-of-line virtual method to provide a 'home' for this class.
   void anchor() override;
 };
 
 extern template class basic_parser<char>;
 
 //--------------------------------------------------
 // PrintOptionDiff
 //
 // This collection of wrappers is the intermediary between class opt and class
 // parser to handle all the template nastiness.
 
 // This overloaded function is selected by the generic parser.
 template <class ParserClass, class DT>
 void printOptionDiff(const Option &O, const generic_parser_base &P, const DT &V,
                      const OptionValue<DT> &Default, size_t GlobalWidth) {
   OptionValue<DT> OV = V;
   P.printOptionDiff(O, OV, Default, GlobalWidth);
 }
 
 // This is instantiated for basic parsers when the parsed value has a different
 // type than the option value. e.g. HelpPrinter.
 template <class ParserDT, class ValDT> struct OptionDiffPrinter {
-  void print(const Option &O, const parser<ParserDT> &P, const ValDT & /*V*/,
+  void print(const Option &O, const basic_parser<ParserDT> &P, const ValDT & /*V*/,
              const OptionValue<ValDT> & /*Default*/, size_t GlobalWidth) {
     P.printOptionNoValue(O, GlobalWidth);
   }
 };
 
 // This is instantiated for basic parsers when the parsed value has the same
 // type as the option value.
 template <class DT> struct OptionDiffPrinter<DT, DT> {
-  void print(const Option &O, const parser<DT> &P, const DT &V,
+  void print(const Option &O, const basic_parser<DT> &P, const DT &V,
              const OptionValue<DT> &Default, size_t GlobalWidth) {
     P.printOptionDiff(O, V, Default, GlobalWidth);
   }
 };
 
 // This overloaded function is selected by the basic parser, which may parse a
 // different type than the option type.
 template <class ParserClass, class ValDT>
 void printOptionDiff(
     const Option &O,
     const basic_parser<typename ParserClass::parser_data_type> &P,
     const ValDT &V, const OptionValue<ValDT> &Default, size_t GlobalWidth) {
-
   OptionDiffPrinter<typename ParserClass::parser_data_type, ValDT> printer;
-  printer.print(O, static_cast<const ParserClass &>(P), V, Default,
-                GlobalWidth);
+  printer.print(O, P, V, Default, GlobalWidth);
 }
 
 //===----------------------------------------------------------------------===//
 // applicator class - This class is used because we must use partial
 // specialization to handle literal string arguments specially (const char* does
 // not correctly respond to the apply method).  Because the syntax to use this
 // is a pain, we have the 'apply' method below to handle the nastiness...
 //

lib/Support/CommandLine.cpp

 PRINT_OPT_DIFF(boolOrDefault)
 PRINT_OPT_DIFF(int)
 PRINT_OPT_DIFF(unsigned)
 PRINT_OPT_DIFF(unsigned long long)
 PRINT_OPT_DIFF(double)
 PRINT_OPT_DIFF(float)
 PRINT_OPT_DIFF(char)
 
-void parser<std::string>::printOptionDiff(const Option &O, StringRef V,
+void parser<std::string>::printOptionDiff(const Option &O, const std::string &V,
                                           const OptionValue<std::string> &D,
                                           size_t GlobalWidth) const {
   printOptionName(O, GlobalWidth);
   outs() << "= " << V;
   size_t NumSpaces = MaxOptWidth > V.size() ? MaxOptWidth - V.size() : 0;
   outs().indent(NumSpaces) << " (default: ";
   if (D.hasValue())
     outs() << D.getValue();

unittests/Support/CommandLineTest.cpp

#else
     (void)name; // Suppress -Wunused-private-field.
 #endif
   }
 
  private:
   const char *const name;
 };
 
-template <typename T>
-class StackOption : public cl::opt<T> {
-  typedef cl::opt<T> Base;
+template <typename T, bool ExternalStorage = false,
+          class ParserClass = cl::parser<T>>
+class StackOption : public cl::opt<T, ExternalStorage, ParserClass> {
+  typedef cl::opt<T, ExternalStorage, ParserClass> Base;
 public:
   // One option...
   template<class M0t>
   explicit StackOption(const M0t &M0) : Base(M0) {}
 
   // Two options...
   template<class M0t, class M1t>
   StackOption(const M0t &M0, const M1t &M1) : Base(M0, M1) {}
for (auto &OM : cl::getRegisteredOptions(*cl::TopLevelSubCommand)) {
     O->setDefault();
   }
 
   EXPECT_TRUE(Opt1 == "true");
   EXPECT_TRUE(Opt2);
   EXPECT_TRUE(Opt3 == 3);
 }
 
+/// Custom dummy parser for int options.
+struct CustomIntParser final : cl::basic_parser<int> {
+  static const int MAGIC_NR = 42;
+  static int printOptionDiffCount;
+  CustomIntParser(cl::Option &O) : cl::basic_parser<int>(O) {}
+  bool parse(cl::Option &O, StringRef ArgName, StringRef Arg, int &Value) {
+    Value = MAGIC_NR;
+    return false;
+  }
+  void printOptionDiff(const cl::Option &O, int V, OptVal Default,
+                       size_t GlobalWidth) const {
+    //printOptionName(O, GlobalWidth);
+    //llvm::outs() << "= *always " << MAGIC_NR << "*\n";
+    printOptionDiffCount++;
+  }
+};
+int CustomIntParser::printOptionDiffCount = 0;
+
+TEST(CommandLineTest, PrintOptionValueWithCustomParser) {
+  cl::ResetCommandLineParser();
+  
+  StackOption<int, false, CustomIntParser> Opt1("opt1", cl::init(0));
+  
+  const char *args[] = {"prog", "-opt1=0"};
+  EXPECT_TRUE(
+    cl::ParseCommandLineOptions(2, args, StringRef(), &llvm::nulls()));
+  
+  EXPECT_TRUE(Opt1 == CustomIntParser::MAGIC_NR);
+  
+  for (auto &OM : cl::getRegisteredOptions(*cl::TopLevelSubCommand)) {
+    cl::Option *O = OM.second;
+    O->printOptionValue(8, true);
+  }
+  
+  EXPECT_TRUE(CustomIntParser::printOptionDiffCount > 0);
+}
+
 }  // anonymous namespace