[RFC] C++11: 'virtual' and 'override'

While doing the conversion of LLVM_OVERRIDE to ‘override’ last night, I noticed that the code base is rather inconsistent on whether the ‘virtual’ keyword is also used when ‘override’ is used.

Should we have a coding standard for this? What’s the preferred direction here? Seems not having ‘virtual’ is less overall text, but not sure how others feel.

Related, should we require use of ‘override’ when methods override a base class method?

I have clang-tidy checks for both though haven’t implemented fixits for them yet.

Thanks,
~Craig

While doing the conversion of LLVM_OVERRIDE to 'override' last night, I
noticed that the code base is rather inconsistent on whether the 'virtual'
keyword is also used when 'override' is used.

Should we have a coding standard for this? What's the preferred direction
here? Seems not having 'virtual' is less overall text, but not sure how
others feel.

My vote: omit virtual if override is used.

(legitimate counterargument: harder to skim/match/read whether a
function is virtual when it's not specified and "override" appears
much later in the declaration)

Related, should we require use of 'override' when methods override a base
class method?

My vote: require override.

I have clang-tidy checks for both though haven't implemented fixits for them
yet.

Cool. There has also been a suggestion that Clang could warn about
omitted override if at least one other member function in the same
class is marked override, which could get us a lot of value built into
the compiler (but not 'all the way', so a clang-tidy check would still
be appropriate).

Hi folks!

> [...] whether the 'virtual' keyword is also used when 'override'
> is used.

> Related, should we require use of 'override' when methods override
> a base class method?

I vote to require "override" and to leave out "virtual".

By the way: This is what Delphi / Free Pascal do,

The Delphi interpretation of "virtual" is to establish a new "slot" (i.e. a new entry in the VMT) whereas "override" re-uses such a slot.

Jasper

> While doing the conversion of LLVM_OVERRIDE to 'override' last night, I
> noticed that the code base is rather inconsistent on whether the
'virtual'
> keyword is also used when 'override' is used.
>
> Should we have a coding standard for this? What's the preferred direction
> here? Seems not having 'virtual' is less overall text, but not sure how
> others feel.

My vote: omit virtual if override is used.

(legitimate counterargument: harder to skim/match/read whether a
function is virtual when it's not specified and "override" appears
much later in the declaration)

One counter-datapoint: Personally, I have on at least one occasion caught
myself not noticing a leading `virtual` and thinking that a method wasn't
overriden because of the missing `override`. I guess the moral is that this
can be pretty adaptable.

FWIW IMO the preferred end state is to have no useless leading `virtual`'s
and using `override` for its intended purpose.

-- Sean Silva

> While doing the conversion of LLVM_OVERRIDE to 'override' last night, I
> noticed that the code base is rather inconsistent on whether the 'virtual'
> keyword is also used when 'override' is used.
>
> Should we have a coding standard for this? What's the preferred direction
> here? Seems not having 'virtual' is less overall text, but not sure how
> others feel.

My vote: omit virtual if override is used.

+1: virtual doesn’t add anything if override is present.

(legitimate counterargument: harder to skim/match/read whether a
function is virtual when it's not specified and "override" appears
much later in the declaration)

One counter-datapoint: Personally, I have on at least one occasion caught myself not noticing a leading `virtual` and thinking that a method wasn't overriden because of the missing `override`. I guess the moral is that this can be pretty adaptable.

FWIW IMO the preferred end state is to have no useless leading `virtual`'s and using `override` for its intended purpose.

-- Sean Silva

> Related, should we require use of 'override' when methods override a base
> class method?

My vote: require override.

+1: override is useful and prevents errors.

While doing the conversion of LLVM_OVERRIDE to ‘override’ last night, I
noticed that the code base is rather inconsistent on whether the ‘virtual’
keyword is also used when ‘override’ is used.

Should we have a coding standard for this? What’s the preferred direction
here? Seems not having ‘virtual’ is less overall text, but not sure how
others feel.

My vote: omit virtual if override is used.

+1: virtual doesn’t add anything if override is present.

(legitimate counterargument: harder to skim/match/read whether a
function is virtual when it’s not specified and “override” appears
much later in the declaration)

One counter-datapoint: Personally, I have on at least one occasion caught myself not noticing a leading virtual and thinking that a method wasn’t overriden because of the missing override. I guess the moral is that this can be pretty adaptable.

FWIW IMO the preferred end state is to have no useless leading virtual’s and using override for its intended purpose.

– Sean Silva

Related, should we require use of ‘override’ when methods override a base
class method?

My vote: require override.

+1: override is useful and prevents errors.

Would it be too much to have clang emit a warning/error if override is missing? I know that sounds crazy and people hate errors which fire too often, but there’s not too much C++11 code out there yet, and so we have a chance to put errors/warnings in now without too much pain. People might just get used to them and think its how code has to be written :slight_smile:

There is already work on a clang-tidy check for this.

We actually have the framework for Chris's asked-for coding convention
checker. Now we get to use it. =D

While doing the conversion of LLVM_OVERRIDE to ‘override’ last night, I
noticed that the code base is rather inconsistent on whether the ‘virtual’
keyword is also used when ‘override’ is used.

Should we have a coding standard for this? What’s the preferred direction
here? Seems not having ‘virtual’ is less overall text, but not sure how
others feel.

My vote: omit virtual if override is used.

+1: virtual doesn’t add anything if override is present.

(legitimate counterargument: harder to skim/match/read whether a
function is virtual when it’s not specified and “override” appears
much later in the declaration)

One counter-datapoint: Personally, I have on at least one occasion caught myself not noticing a leading virtual and thinking that a method wasn’t overriden because of the missing override. I guess the moral is that this can be pretty adaptable.

FWIW IMO the preferred end state is to have no useless leading virtual’s and using override for its intended purpose.

– Sean Silva

Related, should we require use of ‘override’ when methods override a base
class method?

My vote: require override.

+1: override is useful and prevents errors.

Would it be too much to have clang emit a warning/error if override is missing? I know that sounds crazy and people hate errors which fire too often, but there’s not too much C++11 code out there yet, and so we have a chance to put errors/warnings in now without too much pain. People might just get used to them and think its how code has to be written :slight_smile:

Might be a nightmare when including legacy headers, but warnings can always be disabled…

A clang warning for this would be awesome, but it should be off by default. That said, the build of LLVM itself could detect that Clang had this warning and turn it on. I think it would be great to have the makefiles/cmake detect modern clang’s and turn on additional warnings that we can’t inflict on the world by default.

-Chris

It might be reasonable to warn if a class has both a function marked
'override' and a function that overrides but is not marked 'override'.

That could be useful - because it means that the author of the class is at least thinking about override - but having a “coding style” warning of “I always intend to use override” would still be useful.

-Chris

What about tagging the class as requiring override? Could be an
attribute or pragma...

Joerg

Doug (not sure about other Clang owners) is pretty hesitant about
implementing coding style warnings - anything with such a high false
positive rate as to be off by default is assumed to be a non-starter
in Clang (though perhaps things have changed in the years since I last
tested the waters here).

And now that we have something like clang-tidy, it's perhaps less of
an issue... we'll see.

- David

Making it part of clang-tidy would make a lot of sense then! Is there any plans to get clang-tidy running against the llvm/clang codebases regularly, or is it already happening?

-Chris

Alex and others here are actively working on it. I think Craig Topper has
even used a prototype tidy check for this exact issue to do most of the
cleanup. =]

I believe that the “clang-modernize” tool can add “override” in the appropriate places.
http://stackoverflow.com/questions/7293715/is-there-a-tool-to-add-the-override-identifier-to-existing-c-code

— Marshall

It might be reasonable to warn if a class has both a function marked
'override' and a function that overrides but is not marked 'override'.

That could be useful - because it means that the author of the class is at
least thinking about override - but having a "coding style" warning of "I
always intend to use override" would still be useful.

Doug (not sure about other Clang owners) is pretty hesitant about
implementing coding style warnings - anything with such a high false
positive rate as to be off by default is assumed to be a non-starter
in Clang (though perhaps things have changed in the years since I last
tested the waters here).

And now that we have something like clang-tidy, it's perhaps less of
an issue... we'll see.

Making it part of clang-tidy would make a lot of sense then! Is there any
plans to get clang-tidy running against the llvm/clang codebases regularly,
or is it already happening?

I believe that the “clang-modernize” tool can add “override” in the
appropriate places.

overriding - Is there a tool to add the "override" identifier to existing C++ code - Stack Overflow

Can it also delete "virtual" if it has "override"?

clang-modernize can add the ‘override’, but it can’t currently delete ‘virtual’. It will also potentially overflow 80 columns. And if it removed virtual it would fail to align a second line of arguments correctly. So you need modernize and clang-format I guess. Though I’m not sure we want to widespread apply clang-format.

clang-modernize has a -format option that will run clang-format on the code it changes.

Ben

Didn’t realize that. I’ll see if i can figure out how to make it delete the virtual keyword.