[llvm] Revert r231274: "Devirtualize ~parser<T> by making it protected in base classes and making derived classes final"

[hintonda]

Summary

This patch was previously applied in r231221, and reverted in r231254 because it broke self-hosting. It was subsequently fixed and reapplied in r231274. Unfortunately, making the parser<T> classes final prevents inheritance which makes it impossible to implement custom parsers.

Reverting r231221 restores the ability to customize parsers.

[dblaikie]

llvm/include/llvm/Support/CommandLine.h

901–902 ↗

(On Diff #196021)

Might as well make the dtor public and virtual in the basic_parser class & remove all these explicitly virtual dtor definitions in the derived classes, I guess?

[craig.topper]

This seems to reversing r231221 which devirtualized this. What kind of customization are you envisioning? There aren't many methods here. Are you planning to override the parse method but still fall back to the base class version sometimes?

[dblaikie]

In D60955#1474642, @craig.topper wrote:

This seems to reversing r231221 which devirtualized this. What kind of customization are you envisioning? There aren't many methods here. Are you planning to override the parse method but still fall back to the base class version sometimes?

Sorry, yeah, there's some missing context in this review description - it was discussed on llvm-dev here: http://lists.llvm.org/pipermail/llvm-dev/2019-April/131873.html

[hintonda]

In D60955#1474817, @dblaikie wrote:

In D60955#1474642, @craig.topper wrote:

This seems to reversing r231221 which devirtualized this. What kind of customization are you envisioning? There aren't many methods here. Are you planning to override the parse method but still fall back to the base class version sometimes?

Sorry, yeah, there's some missing context in this review description - it was discussed on llvm-dev here: http://lists.llvm.org/pipermail/llvm-dev/2019-April/131873.html

Sorry, I'll update the description. As stated in the thread, I want to customize the std::string parser, specifically the parse method, to treat the string as a yaml file name, and use yaml::Input to populate a struct containing all my options. I also customize the bool parser to tread it as a flag to generate an example yaml file to give users something to start with.

llvm/include/llvm/Support/CommandLine.h

901–902 ↗

(On Diff #196021)

Thanks -- will do. I should have looked at the original diff first instead of just mechanically applying my spot fix to everything else.

[hintonda]

Revert "Recommit r231221: "Devirtualize ~parser<T> by making it protected in base classes and making derived classes final""

[hintonda]

David, I think you're okay with this but just wanted to double check. thanks... don

[dblaikie]

Sorry, the title's multiple negatives confuse me a bit.

Maybe just "Revert r231221: ..." and then in the description explain why it's being reverted, and the history of it being previously reverted/reapplied (including why the revert was rolled back, and why this attempt to revert is different/addresses that problem).

[hintonda]

In D60955#1486217, @dblaikie wrote:

Sorry, the title's multiple negatives confuse me a bit.

No worries. I used git llvm revert to generate it, and just retitled with its suggestion.

Maybe just "Revert r231221: ..." and then in the description explain why it's being reverted, and the history of it being previously reverted/reapplied (including why the revert was rolled back, and why this attempt to revert is different/addresses that problem).

Perfect, I'll do that. Thanks...

llvm/trunk/include/llvm/Support/CommandLine.h

 
 //--------------------------------------------------
 // basic_parser - Super class of parsers to provide boilerplate code
 //
 class basic_parser_impl { // non-template implementation of basic_parser<t>
 public:
   basic_parser_impl(Option &) {}
 
+  virtual ~basic_parser_impl() {}
+
   enum ValueExpected getValueExpectedFlagDefault() const {
     return ValueRequired;
   }
 
   void getExtraOptionNames(SmallVectorImpl<StringRef> &) {}
 
   void initialize() {}
 
public:
 
   // getValueName - Overload in subclass to provide a better default value.
   virtual StringRef getValueName() const { return "value"; }
 
   // An out-of-line virtual method to provide a 'home' for this class.
   virtual void anchor();
 
 protected:
-  ~basic_parser_impl() = default;
-
   // A helper for basic_parser::printOptionDiff.
   void printOptionName(const Option &O, size_t GlobalWidth) const;
 };
 
 // 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 OptVal = OptionValue<DataType>;
 
   basic_parser(Option &O) : basic_parser_impl(O) {}
-
-protected:
-  ~basic_parser() = default;
 };
 
 //--------------------------------------------------
 // parser<bool>
 //
-template <> class parser<bool> final : public basic_parser<bool> {
+template <> class parser<bool> : 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() {}
 
public:
   // An out-of-line virtual method to provide a 'home' for this class.
   void anchor() override;
 };
 
 extern template class basic_parser<bool>;
 
 //--------------------------------------------------
 // parser<boolOrDefault>
-template <>
-class parser<boolOrDefault> final : public basic_parser<boolOrDefault> {
+template <> class parser<boolOrDefault> : public basic_parser<boolOrDefault> {
 public:
   parser(Option &O) : basic_parser(O) {}
 
   // parse - Return true on error.
   bool parse(Option &O, StringRef ArgName, StringRef Arg, boolOrDefault &Val);
 
   enum ValueExpected getValueExpectedFlagDefault() const {
     return ValueOptional;
public:
   void anchor() override;
 };
 
 extern template class basic_parser<boolOrDefault>;
 
 //--------------------------------------------------
 // parser<int>
 //
-template <> class parser<int> final : public basic_parser<int> {
+template <> class parser<int> : 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;
 
   // 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> {
+template <> class parser<unsigned> : 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"; }

 };
 
 extern template class basic_parser<unsigned long>;
 
 //--------------------------------------------------
 // parser<unsigned long long>
 //
 template <>
-class parser<unsigned long long> final
-    : public basic_parser<unsigned long long> {
+class parser<unsigned long long> : 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 "ulong"; }
 
   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;
 };
 
 extern template class basic_parser<unsigned long long>;
 
 //--------------------------------------------------
 // parser<double>
 //
-template <> class parser<double> final : public basic_parser<double> {
+template <> class parser<double> : 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;
 
   // 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> {
+template <> class parser<float> : 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;
 
   // 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> {
+template <> class parser<std::string> : 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;
 
   // 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> {
+template <> class parser<char> : 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;
   }