[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

 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/raw_ostream.h"
 #include <cassert>
 #include <climits>
 #include <cstddef>

     StringRef Name;
     StringRef HelpStr;
   };
   
 public:
+  /// arbitrary spacing for printOptionDiff
+  static const size_t MaxOptWidth = 8;
+
   generic_parser_base(Option &O) : Owner(O) {}
 
   virtual ~generic_parser_base() = default;

 //
 class basic_parser_impl { // non-template implementation of basic_parser<t>
 public:
+  /// arbitrary spacing for printOptionDiff
+  static const size_t MaxOptWidth = generic_parser_base::MaxOptWidth;
+  
   basic_parser_impl(Option &) {}
 
   enum ValueExpected getValueExpectedFlagDefault() const {

 template <class DataType> class basic_parser : public basic_parser_impl {
 public:
   using parser_data_type = DataType;
+  
   using OptVal = OptionValue<DataType>;
+  using OptValParam = typename std::conditional<std::is_scalar<DataType>::value,
+                                                OptVal, const OptVal&>::type;
+  using ValParam = typename std::conditional<std::is_scalar<DataType>::value,
+                                             DataType, const DataType&>::type;
 
   basic_parser(Option &O) : basic_parser_impl(O) {}
 
+  
+  virtual void printOptionDiff(const Option &O, ValParam V, OptValParam Default,
+                               size_t GlobalWidth) const;
+
 protected:
   ~basic_parser() = default;
 };

   // 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;
-
   // An out-of-line virtual method to provide a 'home' for this class.
   void anchor() override;
 };

   // 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;
-
   // An out-of-line virtual method to provide a 'home' for this class.
   void anchor() override;
 };

   // 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;
-
   // An out-of-line virtual method to provide a 'home' for this class.
   void anchor() override;
 };

   // 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;
-
   // An out-of-line virtual method to provide a 'home' for this class.
   void anchor() override;
 };

   // 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;
-
   // An out-of-line virtual method to provide a 'home' for this class.
   void anchor() override;
 };

   // 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;
-
   // An out-of-line virtual method to provide a 'home' for this class.
   void anchor() override;
 };

   // 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;
-
   // An out-of-line virtual method to provide a 'home' for this class.
   void anchor() override;
 };

   // 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;

   // 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;
-
   // An out-of-line virtual method to provide a 'home' for this class.
   void anchor() override;
 };

 // 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);
   }

     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);
 }
 
 //===----------------------------------------------------------------------===//

lib/Support/CommandLine.cpp

   }
 }
 
-static const size_t MaxOptWidth = 8; // arbitrary spacing for printOptionDiff
-
 // printGenericOptionDiff - Print the value of this option and it's default.
 //
 // "Generic" options have each value mapped to a name.

   outs() << "= *unknown option value*\n";
 }
 
-// printOptionDiff - Specializations for printing basic value types.
-//
-#define PRINT_OPT_DIFF(T)                                                      \
-  void parser<T>::printOptionDiff(const Option &O, T V, OptionValue<T> D,      \
-                                  size_t GlobalWidth) const {                  \
-    printOptionName(O, GlobalWidth);                                           \
-    std::string Str;                                                           \
-    {                                                                          \
-      raw_string_ostream SS(Str);                                              \
-      SS << V;                                                                 \
-    }                                                                          \
-    outs() << "= " << Str;                                                     \
-    size_t NumSpaces =                                                         \
-        MaxOptWidth > Str.size() ? MaxOptWidth - Str.size() : 0;               \
-    outs().indent(NumSpaces) << " (default: ";                                 \
-    if (D.hasValue())                                                          \
-      outs() << D.getValue();                                                  \
-    else                                                                       \
-      outs() << "*no default*";                                                \
-    outs() << ")\n";                                                           \
+// explicit basic_parser<DataType>::printOptionDiff template instantiations ::
+
+#define TPLINST_BASIC_PARSER_PRINTOPTDIFF_SCALAR(T)                            \
+  template void basic_parser<T>::printOptionDiff(const Option &O, T V,         \
+                                                 OptVal D, size_t GW) const;
+
+#define TPLINST_BASIC_PARSER_PRINTOPTDIFF_NONSCALAR(T)                         \
+  template void basic_parser<T>::printOptionDiff(const Option &O, const T &V,  \
+                                                 const OptVal &D,              \
+                                                 size_t GW) const;
+
+TPLINST_BASIC_PARSER_PRINTOPTDIFF_SCALAR(bool)
+TPLINST_BASIC_PARSER_PRINTOPTDIFF_SCALAR(boolOrDefault)
+TPLINST_BASIC_PARSER_PRINTOPTDIFF_SCALAR(int)
+TPLINST_BASIC_PARSER_PRINTOPTDIFF_SCALAR(unsigned)
+TPLINST_BASIC_PARSER_PRINTOPTDIFF_SCALAR(unsigned long long)
+TPLINST_BASIC_PARSER_PRINTOPTDIFF_SCALAR(double)
+TPLINST_BASIC_PARSER_PRINTOPTDIFF_SCALAR(float)
+TPLINST_BASIC_PARSER_PRINTOPTDIFF_NONSCALAR(std::string)
+TPLINST_BASIC_PARSER_PRINTOPTDIFF_SCALAR(char)
+
+template<typename DataType>
+void basic_parser<DataType>::printOptionDiff(const Option &O, ValParam V,
+                                             OptValParam D,
+                                             size_t GlobalWidth) const {
+  printOptionName(O, GlobalWidth);
+  std::string Str;
+  {
+    raw_string_ostream SS(Str);
+    SS << V;
   }
+  outs() << "= " << Str;
+  size_t NumSpaces = MaxOptWidth > Str.size() ? MaxOptWidth - Str.size() : 0;
+  outs().indent(NumSpaces) << " (default: ";
+  if (D.hasValue())
+    outs() << D.getValue();
+  else
+    outs() << "*no default*";
+  outs() << ")\n";
+}
 
-PRINT_OPT_DIFF(bool)
-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,
-                                          const OptionValue<std::string> &D,
+void parser<std::string>::printOptionDiff(const Option &O, const std::string &V,
+                                          const OptVal &D,
                                           size_t GlobalWidth) const {
   printOptionName(O, GlobalWidth);
   outs() << "= " << V;