C++11 Feature Proposal: Use override/final instead of OVERRIDE/FINAL

[Daniel Cheng]

What:

Switch from OVERRIDE/FINAL macros to use override/final keywords instead.

Why:

Using virtual can lead to simple mistakes:

class Base {

  virtual void Awesome() { }

};

class Derived : public Base {
  virtual void Awwsome() { }  // Oops!
};

If Awwesome had been annotated with override, the compiler would have caught the fact that Awwsome is not actually overriding anything from Derived's base class.

final is similar to override, but prevents derived classes from overriding the method.

Where:

We already use it extensively throughout Chrome and Blink. It'd be easy to update this.

In addition, I believe we should encourage that developers only use one of virtual|override|final (this is what the Google C++ style guide mandates). There's never a reason to use more than one: override implies virtual, and final implies virtual and override.

Daniel

[Ryan Sleevi]

+1 to OVERRIDE

I was surprised to find out that we had FINAL, and even more, that code was using it. Isn't FINAL more akin to 'inline' (which we presently discourage), in that it's one of those "writers offering hints to the compilers" sort of activities?

That is, I'm aware of the ability for the compiler to prevent overriding of methods annotated FINAL, but the use of FINAL to do so seems to be more emblematic of a design issue (violating Liskov substitution principal / the is-a relationship). Further, we discourage compositional, concrete inheritance, which is where FINAL is most likely to be valuable.

Put differently, I can see benefits to FINAL, but I am very much concerned of the potential of design smell. Which is why I was surprised by FINAL.

[Brett Wilson]

If I recall, the use of FINAL in Blink is a security hardening issue
since there are fewer calls to code with dynamic addresses that can be
corrupted (or something).

Brett

> --
> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev

[Will Harris]

+cevans

Yes, final (or "sealed" in VS parlance) gives definite security benefits as it eliminates indirect calls via vtables.

https://media.blackhat.com/bh-us-12/Briefings/M_Miller/BH_US_12_Miller_Exploit_Mitigation_Slides.pdf slide 13.

Will

[Ryan Sleevi]

Do we have guidelines on when/how to use this?

My concern, especially with your remarks about positive security benefits, is that we end up with "sealed for speed". And the benefits only apply when you're dealing with the concrete derived type, not the indirect base pointer, correct? (I'm assuming WPO/LTO isn't making assumptions about API calls, but that'd be awesome if it was)?

To be clear: I don't have strong objections to this, and security wins are great, I just worry about the cognitive design overhead. I've seen it come up enough in the past with related issues (e.g. publicly inheriting a base class, declaring a public-base method as private in the derived class, then assuming because it's private it's safe to violate the public-base's API contract/assumptions/guarantees, since 'no one' can call it)

[James Robinson]

On Tue, Sep 23, 2014 at 2:25 PM, Daniel Cheng <dch...@chromium.org> wrote:

Where:

We already use it extensively throughout Chrome and Blink. It'd be easy to update this.

This seems fine.  What advice would we give for new code?  Use the lowercase versions always, be consistent with the file, convert as you go?

 

In addition, I believe we should encourage that developers only use one of virtual|override|final (this is what the Google C++ style guide mandates). There's never a reason to use more than one: override implies virtual, and final implies virtual and override.

This seems fine as well, although I think our clang checker will need to be fixed first to recognize these keywords if it doesn't yet.  Could you do an experiment using each of these keywords by themselves in some chromium code and see if the clang checker is happy with it?

The question about how to use the 'final' keyword is interesting as well but I think we should separate that into a separate thread.  We're already using FINAL in several places, so converting those to 'final' seems fine so long as we have a reasonable transition plan.

- James

 

Daniel

[Nico Weber]

Final is sometimes useful to make it clear that a class with virtual methods isn't deleted polymorphically (for example https://codereview.chromium.org/410833002).

It looks like ch.dumez added FINAL to all methods where it could be added in Blink: https://code.google.com/p/chromium/issues/detail?id=333316 I think he no longer works on Blink; maybe eseidel (or someone else on blink-dev) remembers why that was done. I don't think we should do that.

[Peter Kasting]

On Tue, Sep 23, 2014 at 3:15 PM, James Robinson <jam...@chromium.org> wrote:

On Tue, Sep 23, 2014 at 2:25 PM, Daniel Cheng <dch...@chromium.org> wrote:

Where:

We already use it extensively throughout Chrome and Blink. It'd be easy to update this.

This seems fine.  What advice would we give for new code?  Use the lowercase versions always, be consistent with the file, convert as you go?

Could we do a single cleanup pass (using a script) to eliminate the OVERRIDE define and change to "override" everywhere?  It would be nice not to be inconsistent on this one.

PK

[Daniel Cheng]

I agree--we should just do a cleanup pass and flip everything over. We'll just have to make sure the plugin can handle both variations for the time being.

Daniel

[Ryan Sleevi]

Agreed, sorry for confusing the discussion.

Big +1 to nuking OVERRIDE and FINAL in favor of "override" and "final" now that it's formally supported everywhere, and to updating our plugin if necessary.

Regarding widespread use of "final", I would hope that guidance is provided, lest it be seen as "inline for speed" or as a way of making an "API boundary that isn't"

[Peter Kasting]

On Tue, Sep 23, 2014 at 3:25 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

Big +1 to nuking OVERRIDE and FINAL in favor of "override" and "final" now that it's formally supported everywhere, and to updating our plugin if necessary.

Incidentally, if we're going to use a tool to convert all OVERRIDE to override, I suggest the tool also remove the "virtual" keyword in such cases in the same pass, so we're consistent from the beginning and don't have to do two cleanup passes.

PK 

[Ryan Hamilton]

Will we follow the Google style guide which prohibits virtual when override is present? For example:

  void Foo() override;

not

  virtual void Foo() override;

On Tue, Sep 23, 2014 at 2:25 PM, Daniel Cheng <dch...@chromium.org> wrote:

--

[Hendrik]

Incidentally, if we're going to use a tool to convert all OVERRIDE to override, I suggest the tool also remove the "virtual" keyword in such cases in the same pass, so we're consistent from the beginning and don't have to do two cleanup passes.

PK 

Removing the virtual would be my preference as well.  It becomes redundant when you have override. 

[Peter Kasting]

On Tue, Sep 23, 2014 at 3:28 PM, Ryan Hamilton <r...@chromium.org> wrote:

Will we follow the Google style guide which prohibits virtual when override is present? For example:

  void Foo() override;

not

  virtual void Foo() override;

See the last portion of the mail you replied to:

On Tue, Sep 23, 2014 at 2:25 PM, Daniel Cheng <dch...@chromium.org> wrote:

In addition, I believe we should encourage that developers only use one of virtual|override|final (this is what the Google C++ style guide mandates). There's never a reason to use more than one: override implies virtual, and final implies virtual and override.

(I agree with Daniel, hence my proposal to have our cleanup tool do this.)

PK 

[Daniel Cheng]

Yes, that was part of my original email. Developers should only use one of {virtual,override,final}.

And yes, this hypothetical script to cleanup usages should remove the virtual keyword as well.

Daniel

On Tue, Sep 23, 2014 at 3:28 PM, Ryan Hamilton <r...@chromium.org> wrote:

[Elliot Glaysher (Chromium)]

Having only one of {virtual, override} may also require changing the style checking plugin. I remember adding a "virtual" check years ago.

On Tue, Sep 23, 2014 at 3:28 PM, Ryan Hamilton <r...@chromium.org> wrote:

[James Robinson]

The plugin won't care about the spelling of 'override' or 'OVERRIDE', since it runs after the preprocessor.  The thing that the plugin might get confused about is saying 'void foo() override;' without the 'virtual' keyword.

- James

[Daniel Cheng]

I did some local testing (results at https://gist.github.com/anonymous/2481cbdfba25aea03931). The plugin currently always requires virtual on virtual methods, even if already annotated with override/final. It also requires override to be specified with final. So the following things will need to change in the plugin:

- Drop 'virtual' requirement if method is annotated with override or final.

- Drop 'override' requirement if method is annotated with final.

- In addition, the plugin will need to start enforcing only one of {virtual,override,final}, but will have to be controlled by a flag that we can switch on after the cleanup is complete.

Daniel

[James Robinson]

On Tue, Sep 23, 2014 at 9:33 PM, Elliott Sprehn <esp...@google.com> wrote:

Last time we tried this override and final produced different warnings so you really did need both. Has that changed? For example if you remove the super class method entirely does it fail to compile if you only have final and not virtual and override?

That would be the job of our clang checker to enforce, which as has been mentioned by Daniel doesn't support using these keywords in the way we want.  When we fix that declaring final on a function that does not actually override something should produce an error just like declaring override on a function that does not override something does today.  Somebody will have to go patch the clang checker before any of the rest of this can happen, however.

- James

In blink final is an assertion from the author that they're the leaf for that method or class. If you need to extend something you're welcome to remove it and mark your new method final (or more likely your whole class). Having it is important for performance (and not harmful like inline) and also security. We should not change the encouragement of marking all leaves in blink final.

--

[Daniel Cheng]

dcheng-macbookpro:~ dcheng$ cat test.cc

class Base {

  virtual void F();

};

class Derived {

  void F2() final;

};

dcheng-macbookpro:~ dcheng$ clang++ -std=c++11 test.cc

test.cc:6:13: error: only virtual member functions can be marked 'final'

  void F2() final;

            ^~~~~

1 error generated.

​So this is baked into the compiler already.

Daniel​

[Daniel Cheng]

(Err I typoed that example a bit, but the point is still the same).

Daniel

[Nico Weber]

Summary, as I understand it:

* There are no concerns about using lowercase override / final, so we should allow it for new code.

* We're going to mass-replace existing OVERRIDE / FINAL with lower-case variants. dcheng is working on this.

* We're going to change the style plugin so that it checks for one of {virtual,override,final}. esprehn had concerns about this, but it sounds like they have been addressed.

* Blink uses final in a different way than the rest of chromium. That's fine.

dcheng, do you want to prepare a CL to move final and override to the table with allowed stuff in src/styleguide/c++/c++11.html?

[James Robinson]

On Wed, Sep 24, 2014 at 1:48 PM, Nico Weber <tha...@chromium.org> wrote:

* Blink uses final in a different way than the rest of chromium. That's fine.

What's the difference?  My understanding is the current usage in blink is the same as the expected usage in Chromium.

- James

[Nico Weber]

Blink marks lots of methods final (see https://code.google.com/p/chromium/issues/detail?id=333316). My understanding from the "when to use final" thread was that we don't want to do this in chromium (?)

[Peter Kasting]

On Wed, Sep 24, 2014 at 1:51 PM, Nico Weber <tha...@chromium.org> wrote:

Blink marks lots of methods final (see https://code.google.com/p/chromium/issues/detail?id=333316). My understanding from the "when to use final" thread was that we don't want to do this in chromium (?)

I'd like to better understand Blink's rules on when final should be used; once those are clear, I think we should harmonize Blink and Chromium here (I have no opinion about what the harmonized version looks like).

If there are good reasons to use final or not use final in particular cases, those reasons likely apply equally to the Blink and Chromium codebase, so let's avoid divergence.

PK

[James Robinson]

I think we should use final everywhere it makes sense, which is the same as the rule in Blink.

- James

[Ryan Sleevi]

> --
> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev

+2

If the security wins are meaningful, to me that overrides any of the design concerns.

And I say that as a person who seriously cares about design.

I just hope we provide guidance on when/how the security gains are meaningful, for all codebases.

[Peter Kasting]

OK, what does "make sense" mean?  All virtual functions that don't have further overrides (meaning that most instances of virtual/override we have in the codebase today would become final)?

PK 

[Daniel Cheng]

​Just FYI, there are no plans to automatically rewrite virtual functions with no overrides to be final. That gets rather tricky.

Daniel​

[Daniel Cheng]

I started to draft a patch, but two questions:

1) Do we want to address style at all, or just assume that people will delegate to the Google C++ style guide?

2) We've never annotated our destructors with override or final, but the Google C++ style guide seems to indicate this is preferred. Is this the approach we want to take as well?

Daniel

On Wed, Sep 24, 2014 at 1:48 PM, Nico Weber <tha...@chromium.org> wrote:

[Nico Weber]

On Wed, Sep 24, 2014 at 2:33 PM, Daniel Cheng <dch...@chromium.org> wrote:

I started to draft a patch, but two questions:

1) Do we want to address style at all, or just assume that people will delegate to the Google C++ style guide?

2) We've never annotated our destructors with override or final, but the Google C++ style guide seems to indicate this is preferred. Is this the approach we want to take as well?

I'd just say "Using override instead of OVERRIDE is recommended for new code" for now, and then add more things later, once the plugin actually supports them.

[Peter Kasting]

On Wed, Sep 24, 2014 at 2:33 PM, Daniel Cheng <dch...@chromium.org> wrote:

I started to draft a patch, but two questions:

1) Do we want to address style at all, or just assume that people will delegate to the Google C++ style guide?

Chromium style assumes the Google style guide and then notes exceptions.  So we should only address style insofar as we want to differ from the Google style guide.

PK

[Nico Weber]

On Wed, Sep 24, 2014 at 2:42 PM, Nico Weber <tha...@chromium.org> wrote:

On Wed, Sep 24, 2014 at 2:33 PM, Daniel Cheng <dch...@chromium.org> wrote:

I started to draft a patch, but two questions:

1) Do we want to address style at all, or just assume that people will delegate to the Google C++ style guide?

2) We've never annotated our destructors with override or final, but the Google C++ style guide seems to indicate this is preferred. Is this the approach we want to take as well?

I'd just say "Using override instead of OVERRIDE is recommended for new code" for now, and then add more things later, once the plugin actually supports them.

This is now live: http://chromium-cpp.appspot.com/ (doesn't link to the google style guide yet, since the plugin doesn't permit the style recommended there yet. It does link to a tracking bug for making that work.)

[Daniel Cheng]

Since this has surprised some people, I wanted to make sure there aren't broader objections from Chromium developers before completing this migration.

Style guide [http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance]:

Explicitly annotate overrides of virtual functions or virtual destructors with an override or (less frequently) final specifier.

Today, Chromium only uses 'virtual' for destructors. Does anyone have objections to just using the Google C++ style rule here?

Daniel

[James Robinson]

I think we should standardize on the Google C++ rules here as quickly as feasible.

- James

[Daniel Cheng]

OK, if I don't hear any objections by Monday 5PM PST, I will be TBRing the changes to standardize on the Google C++ style rules.

Daniel

[Peter Kasting]

On Sun, Oct 19, 2014 at 8:44 PM, Daniel Cheng <dch...@chromium.org> wrote:

Since this has surprised some people, I wanted to make sure there aren't broader objections from Chromium developers before completing this migration.

Style guide [http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance]:

Explicitly annotate overrides of virtual functions or virtual destructors with an override or (less frequently) final specifier.

Today, Chromium only uses 'virtual' for destructors. Does anyone have objections to just using the Google C++ style rule here?

To be clear, you're saying that in this transformation example:

class Base {

  virtual ~Base();

  virtual void foo();

};

class Derived {

  ~Derived() override;  // Used to be "virtual ~Derived();"

  void foo() override;  // Used to be "virtual void foo();"

};

...the change to foo() is not surprising people, but the change to ~Derived() is?

I admit that somehow to me ~Derived() doesn't "feel like" an override of ~Base(), but the style guide seems explicit, so if this is what we're talking about, I support using the Google rules.

PK

[Daniel Cheng]

Yes, several people have been surprised that destructors should be marked override.

Daniel

[Alex Vakulenko]

~Derived() and ~Base() is just a C++ syntax to denote a class's destructor and if the base class has virtual destructor, the derived class's destructor is overriding it. I think it is totally expected/advised to use "override" on destructors as well for the same compiler assistance/checking as with regular virtual methods.

[Daniel Bratell]

On Mon, 20 Oct 2014 23:02:52 +0200, Alex Vakulenko <avaku...@chromium.org> wrote:

~Derived() and ~Base() is just a C++ syntax to denote a class's destructor and if the base class has virtual destructor, the derived class's destructor is overriding it. I think it is totally expected/advised to use "override" on destructors as well for the same compiler assistance/checking as with regular virtual methods.

But ~Derived() is not overriding (in English) ~Base(), it's complementing it. This is very different from normal methods where adding a new method replaces them so I can easily see why people think it's the not very logical.

There is also no risk of misspelling a destructor or getting the arguments or return value incorrect so the only remaining reason to have any kind of markup is to verify that it is a virtual destructor. Showing that something is a virtual destructor is easiest with the word "virtual".

/daniel

[Alex Vakulenko]

You have a virtual ~Base(); and virtual ~Derived(). Someone goes in and removes "virtual" from ~Base() without giving it a second though. ~Derived() is still marked as virtual, so the code compiles and everything "works" but run-time functionality is broken. If ~Derived() is marked "override" instead, removing "virtual" from ~Base() instantly breaks the build.

And if you think of what virtual functions actually are (in terms of virtual table entries), ~Derived() actually replaces the entry for ~Base() in the virtual table. The fact that the destructor of derived class automatically calls a destructor of base class implicitly doesn't change the overriding nature of the functions.

[James Robinson]

On Mon, Oct 20, 2014 at 2:39 PM, Daniel Bratell <bra...@opera.com> wrote:

Either way, it's confusing.  The 'virtual' annotation does not connotate that the destructor of the base class must be virtual for the derived class to behave as expected, so that doesn't capture everything that you might want to know.  It's true that the execution model for virtual destructors is different from other virtual functions, but that's C++ for you.  Neither label applies in the same way it would to other functions and the tooling checks that we want are more consistent with 'override', so we should go with that.

- James

/daniel

[Daniel Cheng]

The main argument I've seen for marking destructors override is it prevents something like this:

class Base {

  ~Base() {}

  virtual void Foo() = 0;
};

class Derived : public Base {

  virtual ~Derived();

  void Foo() override {}

  // Other stuff...
};

void Bar() {

  Base* b = new Derived;

  delete b;  // Undefined.

}

On Mon, Oct 20, 2014 at 2:39 PM, Daniel Bratell <bra...@opera.com> wrote:

[David Benjamin]

Wouldn't that case be better covered by a rule like: if Base has a non-override virtual method (so we expect there to be cases where Base* is secretly a Derived*), it should either have a virtual destructor or a protected one? Seems you could otherwise just as easily forget to add the virtual on Derived.

Or the simpler: if there's any virtual method, the destructor must also be virtual, since you're paying for the vtable already anyway. I could have sworn we already had that rule, but maybe I'm hallucinating.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Alex Vakulenko]

Having virtual methods is not the main indication for having virtual destructor. A class might have no virtual methods and can still be derived from and used as (and, most importantly, destroyed trough) a pointer to the base class.

I personally follow this rule: if a class can be a base for another class, it must have a virtual destructor (user-provided or default). Otherwise the class must be marked "final" so no one can derive from it. Seems to be working well...

[Peter Kasting]

On Mon, Oct 20, 2014 at 3:00 PM, David Benjamin <davi...@chromium.org> wrote:

Wouldn't that case be better covered by a rule like: if Base has a non-override virtual method (so we expect there to be cases where Base* is secretly a Derived*), it should either have a virtual destructor or a protected one? Seems you could otherwise just as easily forget to add the virtual on Derived.

Or the simpler: if there's any virtual method, the destructor must also be virtual, since you're paying for the vtable already anyway. I could have sworn we already had that rule, but maybe I'm hallucinating.

The style guide actually says that classes with virtual methods should have virtual destructors, so in some sense we do have that kind of rule.

The style guide also says to mark ~Derived() override in this case.  Arguing why a simple "virtual ~Derived()" declaration tells people what they need to know somewhat misses the point, in my opinion; rather, I'd like to hear arguments why "~Derived() override" is actively worse (e.g. less safe), and/or why something about Chromium code differs from Google code in a way that affects whether we should follow this rule.  I've not yet seen either.

PK

[Daniel Cheng]

On Mon, Oct 20, 2014 at 3:28 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Mon, Oct 20, 2014 at 3:00 PM, David Benjamin <davi...@chromium.org> wrote:

Wouldn't that case be better covered by a rule like: if Base has a non-override virtual method (so we expect there to be cases where Base* is secretly a Derived*), it should either have a virtual destructor or a protected one? Seems you could otherwise just as easily forget to add the virtual on Derived.

Or the simpler: if there's any virtual method, the destructor must also be virtual, since you're paying for the vtable already anyway. I could have sworn we already had that rule, but maybe I'm hallucinating.

The style guide actually says that classes with virtual methods should have virtual destructors, so in some sense we do have that kind of rule.

​It doesn't seem to be enforced by a compiler flag: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chromeos/file_system_provider/observer.h&l=18

Is there a warning we can/should enable for this?​

[Peter Kasting]

On Mon, Oct 20, 2014 at 3:31 PM, Daniel Cheng <dch...@chromium.org> wrote:

On Mon, Oct 20, 2014 at 3:28 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Mon, Oct 20, 2014 at 3:00 PM, David Benjamin <davi...@chromium.org> wrote:

Wouldn't that case be better covered by a rule like: if Base has a non-override virtual method (so we expect there to be cases where Base* is secretly a Derived*), it should either have a virtual destructor or a protected one? Seems you could otherwise just as easily forget to add the virtual on Derived.

Or the simpler: if there's any virtual method, the destructor must also be virtual, since you're paying for the vtable already anyway. I could have sworn we already had that rule, but maybe I'm hallucinating.

The style guide actually says that classes with virtual methods should have virtual destructors, so in some sense we do have that kind of rule.

​It doesn't seem to be enforced by a compiler flag: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chromeos/file_system_provider/observer.h&l=18

Is there a warning we can/should enable for this?​

I think in the past we've said that in a case like this a protected non-virtual destructor would also be acceptable.  Having no explicit destructor at all, as this code does, is probably wrong, though.

PK

[Daniel Bratell]

On Mon, 20 Oct 2014 23:47:49 +0200, Alex Vakulenko <avaku...@chromium.org> wrote:

You have a virtual ~Base(); and virtual ~Derived(). Someone goes in and removes "virtual" from ~Base() without giving it a second though. ~Derived() is still marked as virtual, so the code compiles and everything "works" but run-time functionality is broken. If ~Derived() is marked "override" instead, removing "virtual" from ~Base() instantly breaks the build.

I think this is the one reason to use override, even if it's weird.

/Daniel

[Victor Khimenko]

The question is: is it "weird enough" to not use it? "Yoda contitionals" are also safer, but they looks so weird that people go to great lengths (checks in compiler, etc) to avoid them.

[David Michael]

I'll throw my 2 cents in. We should use override rather than virtual on appropriate destructors. 

1) It's safer, Alex noted and Daniel quoted.

2) It's consistent with Google style, as Peter pointed out.

The weirdness is minor compared to those benefits, IMO. We'll get used to it.