Requiring OVERRIDE?

[Avi Drissman]

During my refactoring work, I've found OVERRIDE annotations to be useful. (More precisely, they've saved my rear several times.) If it were up to me, I'd make them mandatory. Is this an opinion shared?

I've filed a bug: crbug.com/104314 and am looking into putting support for this into our clang style plugin. Let me know if you object.

Avi

[Jói Sigurðsson]

+1 for making them mandatory, they are extremely useful.

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

[Ben Goodger (Google)]

+1 Also.

-Ben

2011/11/15 Jói Sigurðsson <j...@chromium.org>

[Alexei Svitkine]

+1

[Paweł Hajdan, Jr.]

On Tue, Nov 15, 2011 at 18:13, Avi Drissman <a...@google.com> wrote:

During my refactoring work, I've found OVERRIDE annotations to be useful. (More precisely, they've saved my rear several times.) If it were up to me, I'd make them mandatory. Is this an opinion shared?

I also think they're very useful. If we can catch "stupid mistakes" with a compiler, I think we should do it.

By the way, it'd be also great to have some annotation that says "this is not an override", to make sure one is not accidentally overriding parent class' method (there might be even some old discussion about this).

[Alexei Svitkine]

By the way, it'd be also great to have some annotation that says "this is not an override", to make sure one is not accidentally overriding parent class' method (there might be even some old discussion about this).

If it was mandatory and checked a by tool, then the tool would tell you you're missing the OVERRIDE in this case. So explicit annotation wouldn't be needed.

-Alexei

[Paweł Hajdan, Jr.]

Awesome! One more reason to +1 this.

[James Robinson]

On Tue, Nov 15, 2011 at 9:13 AM, Avi Drissman <a...@google.com> wrote:

During my refactoring work, I've found OVERRIDE annotations to be useful. (More precisely, they've saved my rear several times.) If it were up to me, I'd make them mandatory. Is this an opinion shared?

There's one case where you do not want to enforce this - when overriding a function defined in a header outside of the Chromium repo itself.  For example, when implementing an interface defined in the WebKit API.  Adding or requiring OVERRIDE for that case makes it extremely difficult to do rolls and two-sided commits since you can't do very useful things like land an implementation in chromium before landing the interface change in WebKit.

If the .h and the .cc are both in the chromium repo then I strongly support making this required.

- James

I've filed a bug: crbug.com/104314 and am looking into putting support for this into our clang style plugin. Let me know if you object.

Avi

--

[Alexei Svitkine]

There's one case where you do not want to enforce this - when overriding a function defined in a header outside of the Chromium repo itself.  For example, when implementing an interface defined in the WebKit API.  Adding or requiring OVERRIDE for that case makes it extremely difficult to do rolls and two-sided commits since you can't do very useful things like land an implementation in chromium before landing the interface change in WebKit.

In that example, you would still be able to land the interface in WebKit first and land the implementation in Chromium after the WebKit roll.

The real problem would be the case of renaming a method in a WebKit interface.

If you rename it in WebKit, then the roll won't succeed because Chromium will use the old name with OVERRIDE. If you rename it in Chromium first, then you can't put OVERRIDE there because the new interface doesn't yet exist. But if you don't put OVERRIDE and then add the WebKit interface, then the WebKit roll won't work because it will complain that your method doesn't have OVERRIDE set.

-Alexei

[Peter Kasting]

On Tue, Nov 15, 2011 at 9:13 AM, Avi Drissman <a...@google.com> wrote:

During my refactoring work, I've found OVERRIDE annotations to be useful. (More precisely, they've saved my rear several times.) If it were up to me, I'd make them mandatory. Is this an opinion shared?

The style guide is unequivocal: "When overridding a virtual method in a parent class, use the OVERRIDE annotation (see compiler_specific.h) to mark it as overriding the parent."  Therefore I thought this was already required.  I support adding checks to ensure people do this.

PK

[Dominic Mazzoni]

On Tue, Nov 15, 2011 at 10:40 AM, James Robinson <jam...@google.com> wrote:

On Tue, Nov 15, 2011 at 9:13 AM, Avi Drissman <a...@google.com> wrote:

During my refactoring work, I've found OVERRIDE annotations to be useful. (More precisely, they've saved my rear several times.) If it were up to me, I'd make them mandatory. Is this an opinion shared?

There's one case where you do not want to enforce this - when overriding a function defined in a header outside of the Chromium repo itself.  For example, when implementing an interface defined in the WebKit API.  Adding or requiring OVERRIDE for that case makes it extremely difficult to do rolls and two-sided commits since you can't do very useful things like land an implementation in chromium before landing the interface change in WebKit.

If the .h and the .cc are both in the chromium repo then I strongly support making this required.

What if there was a way to bypass the OVERRIDE check for a particular line of code, so that we could make the check really aggressive but still provide a way to get around it *temporarily* for something like a WebKit rename or other unanticipated future issues? Kind of line //nolint for lint checkers.

- Dominic

[James Robinson]

That would add an extra step to multi-sided rolls (landing an inline suppression in Chromium code).  The point is that multi-sided rolls are already extremely difficult and we should not add extra steps to that process.

- James

- Dominic

[Avi Drissman]

On Tue, Nov 15, 2011 at 1:40 PM, James Robinson <jam...@google.com> wrote:

There's one case where you do not want to enforce this - when overriding a function defined in a header outside of the Chromium repo itself.

I'm OK with this. Is there a good way to semantically determine this? In the clang plugin, I have access to the AST but I'm not sure what else I can get my hands on.

Avi 

[James Robinson]

Can you check if the function is defined in a class in the namespace WebKit?  We shouldn't be defining anything in that namespace inside the Chromium repo.  That would only handle WebKit, I'm not sure what other libraries (if any) we have this issue for.

- James

Avi 

[David Michael]

+1 on making it required for virtual functions in chromium.

You can also access the source location of a declaration in Clang. Maybe you can ignore virtual functions that override something from third_party (or just third_party/WebKit).

See clang::SourceLocation. There's an example of using it (which may be out-of-date) in ppapi/tests/clang/print_names_and_sizes.cc

-Dave

[John Abd-El-Malek]

+1 this is also very useful when doing refactorings, and not having it has caused bugs when renaming/moving functions.

[John Abd-El-Malek]

(hit send too soon!)

+1 also for not enforcing it for WebKit implementations, as this is extremely useful when doing two-sided patches.

[Avi Drissman]

I have on my Mac a working modification to our Clang style plugin that complains on overriding without OVERRIDE yet ignores overriding of any member function whose originating class is in the WebKit namespace. I trust that alleviates all concerns.

I'm currently spewing changelists to add OVERRIDE where appropriate. I hope to get the style plugin change in reasonably soon.

Avi

[Martin Kosiba]

Could you provide some concrete examples?

IMHO this would be most useful when changing any of the observer/
Client class definitions. In that case I'd prefer to see the base
class provide _pure_ virtual methods (and not virtual methods with
empty implementations). This way you'd get a compile error (on every
supported compiler) whenever you forget to update a child of the
client class. The only caveat is that if a client wants to listen to
only one or two callbacks it has to provide empty stubs for everything
else - was this the main motivation for writing the base classes this
way?

[Avi Drissman]

Exactly. If you look at something like TabContentsObserver (or worse, TabContentsDelegate), you'll see that there are just too many functions to require all implementers to override. Yes, making the base class's declaration pure would fix it, but with OVERRIDE we get to have the compiler help without paying the price of writing lots of empty overriders. (The downside is we don't get automated help when we add a function.)

Having OVERRIDE has helped me a lot in refactoring. The reason I bring this up is that I just rejiggered the Sad Tab workings, and added a termination status to the TCO's RenderViewGone. I had done a project-wide find to find all uses, but a different observer also has a RenderViewGone function, and in the list of all of them I missed one. OVERRIDE to the rescue again.

Avi

[Martin Kosiba]

Makes sense, thanks for explaining!

[David Michael]

About the WebKit exemption:

It definitely makes sense to not require OVERRIDE, but at the same time, it would be nice if a breaking WebKit API change would show up in an obvious way.

Should we still prefer to use OVERRIDE for implementing fairly stable APIs, and only omit it if we expect churn in the API or are preparing for a two-sided change? Or are we suggesting you never use OVERRIDE when overriding WebKit functions?

I ask because this came up in a code review. My preference is to add it where we think it makes sense (e.g., a stable API). This would be a judgment call left up to the developer & reviewer(s).

-Dave

[Avi Drissman]

I'm good with preferring it be there to help catch issues. I wrote the Clang plugin to not enforce it.

Avi