[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

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

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/include/llvm/Support/CommandLine.h

 
 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() {}
 
   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;
 
   // An out-of-line virtual method to provide a 'home' for this class.
   void anchor() override;
+
+  virtual ~parser() = default;
 };
 
 extern template class basic_parser<bool>;
 
 //--------------------------------------------------
 // parser<boolOrDefault>
 template <>
-class parser<boolOrDefault> final : public basic_parser<boolOrDefault> {
+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;
   }
 
   // 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;
+
+  virtual ~parser() = default;
 };
 
 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;
+
+  virtual ~parser() = default;
 };
 
 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"; }
 
   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;
+
+  virtual ~parser() = default;
 };
 
 extern template class basic_parser<unsigned>;
 
 //--------------------------------------------------
 // parser<unsigned long long>
 //
 template <>
-class parser<unsigned long long> final
+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 "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;
+
+  virtual ~parser() = default;
 };
 
 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;
+
+  virtual ~parser() = default;
 };
 
 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;
+
+  virtual ~parser() = default;
 };
 
 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;
+
+  virtual ~parser() = default;
 };
 
 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;
   }
 
   // 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;
+
+  virtual ~parser() = default;
 };
 
 extern template class basic_parser<char>;
 
 //--------------------------------------------------
 // PrintOptionDiff
 //
 // This collection of wrappers is the intermediary between class opt and class