[llvm-commits] [llvm] r163996 - /llvm/trunk/include/llvm/TableGen/Record.h

[Sean Silva]

I personally think just LLVM_OVERRIDE/LLVM_FINAL is enough. My pretty
weak reason is that I don't remember any time when I saw a `virtual`
in front and it helped me by making me understand the situation.
However, I can recall very recently reading a method and asked myself
"is this overriden from a base class?", and the first thing I looked
for was `override` (or a C++11-prep macro in its place). I regularly
notice the trailing `const` on methods when reading the signatures, so
it's not like the trailing position is a black hole either.

Also, while I think that it is true that people are used to seeing
`virtual` to mark an override, that will be phased out over time
anyway as C++11 becomes more widespread. I think the code should opt
for being more modern. I anticipate that years down the road, once
C++11 is the norm, a warning will be introduced that warns over
"pointless `virtual`, use `override` instead".

--Sean Silva

On Sun, Sep 16, 2012 at 4:45 PM, Craig Topper <craig.topper at gmail.com> wrote:
> I have a patch to add LLVM_OVERRIDE and LLVM_FINAL that I'll commit shortly.
> Do we want virtual and override or just override? People are used to seeing
> virtual and know what it means. Just override at the end of the line is a
> little less obvious. We obviously have inconsistent usage of virtuals all
> over the tree today so would probably good to have a standard.
>
>
> On Sunday, September 16, 2012, Craig Topper wrote:
>>
>>
>>
>> On Sunday, September 16, 2012, Sean Silva wrote:
>>
>> Maybe we should add LLVM_OVERRIDE to our C++11-prep arsenal? This
>> really isn't what the `virtual` keyword is meant for...
>>
>> --Sean Silva
>>
>> On Sun, Sep 16, 2012 at 3:39 AM, Craig Topper <craig.topper at gmail.com>
>> wrote:
>> > Author: ctopper
>> > Date: Sun Sep 16 02:39:55 2012
>> > New Revision: 163996
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=163996&view=rev
>> > Log:
>> > Add explicit virtual keywords for methods that override base class.
>> >
>> > Modified:
>> >     llvm/trunk/include/llvm/TableGen/Record.h
>> >
>> > Modified: llvm/trunk/include/llvm/TableGen/Record.h
>> > URL:
>> > http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/TableGen/Record.h?rev=163996&r1=163995&r2=163996&view=diff
>> >
>> > ==============================================================================
>> > --- llvm/trunk/include/llvm/TableGen/Record.h (original)
>> > +++ llvm/trunk/include/llvm/TableGen/Record.h Sun Sep 16 02:39:55 2012
>> > @@ -152,9 +152,9 @@
>> >    virtual Init *convertValue(   VarInit *VI) { return
>> > RecTy::convertValue(VI);}
>> >    virtual Init *convertValue( FieldInit *FI) { return
>> > RecTy::convertValue(FI);}
>> >
>> > -  std::string getAsString() const { return "bit"; }
>> > +  virtual std::string getAsString() const { return "bit"; }
>> >
>> > -  bool typeIsConvertibleTo(const RecTy *RHS) const {
>> > +  virtual bool typeIsConvertibleTo(const RecTy *RHS) const {
>> >      return RHS->baseClassOf(this);
>> >    }
>> >    virtual bool baseClassOf(const BitRecTy    *RHS) const { return true;
>> > }
>> > @@ -195,9 +195,9 @@
>> >    virtual Init *convertValue(   VarInit *VI) { return
>> > RecTy::convertValue(VI);}
>> >    virtual Init *convertValue( FieldInit *FI) { return
>> > RecTy::convertValue(FI);}
>> >
>> > -  std::string getAsString() const;
>> > +  virtual std::string getAsString() const;
>> >
>> > -  bool typeIsConvertibleTo(const RecTy *RHS) const {
>> > +  virtual bool typeIsConvertibleTo(const RecTy *RHS) const {
>> >      return RHS->baseClassOf(this);
>> >    }
>> >    virtual bool baseClassOf(const BitRecTy    *RHS) const { return Size
>> > == 1; }
>> > @@ -237,9 +237,9 @@
>> >    virtual Init *convertValue(   VarInit *VI) { return
>> > RecTy::convertValue(VI);}
>> >    virtual Init *convertValue( FieldInit *FI) { return
>> > RecTy::convertValue(FI);}
>> >
>> > -  std::string getAsString() const { return "int"; }
>> > +  virtual std::string getAsString() const { return "int"; }
>> >
>> > -  bool typeIsConvertibleTo(const RecTy *RHS) const {
>> > +  virtual bool typeIsConvertibleTo(const RecTy *RHS) const {
>> >      return RHS->baseClassOf(this);
>> >    }
>> >
>> > @@ -278,9 +278,9 @@
>> >    virtual Init *convertValue(   VarInit *VI) { return
>> > RecTy::convertValue(VI);}
>> >    virtual Init *convertValue( FieldInit *FI) { return
>> > RecTy::convertValue(FI);}
>> >
>> > -  std::string getAsString() const { return "string"; }
>> > +  virtual std::string getAsString() const { return "string"; }
>> >
>> > -  bool typeIsConvertibleTo(const RecTy *RHS) const {
>> > +  virtual bool typeIsConvertibleTo(const RecTy *RHS) const {
>> >      return RHS->baseClassOf(this);
>> >    }
>> >
>> > @@ -322,9 +322,9 @@
>> >    virtual Init *convertValue(   VarInit *VI) { return
>> > RecTy::convertValue(VI);}
>> >    virtual Init *convertValue( FieldInit *FI) { return
>> > RecTy::convertValue(FI);}
>> >
>> > -  std::string getAsString() const;
>> > +  virtual std::string getAsString() const;
>> >
>> > -  bool typeIsConvertibleTo(const RecTy *RHS) const {
>> > +  virtual bool typeIsConvertibleTo(const RecTy *RHS) const {
>> >      return RHS->baseClassOf(this);
>> >    }
>> >
>> > @@ -363,9 +363,9 @@
>> >    virtual Init *convertValue(   VarInit *VI) { return
>> > RecTy::convertValue(VI);}
>> >    virtual Init *convertValue( FieldInit *FI) { return
>> > RecTy::convertValue(FI);}
>> >
>> > -  std::string getAsString() const { return "dag"; }
>> > +  virtual std::string getAsString() const { return "dag"; }
>> >
>> > -  bool typeIsConvertibleTo(const RecTy *RHS) const {
>> > +  virtual bool typeIsConvertibleTo(const RecTy *RHS) const {
>> >      return RHS->baseClassOf(this);
>> >    }
>> >
>> > @@ -407,9 +407,9 @@
>> >
>>
>> --
>> ~Craig
>
>
>
> --
> ~Craig