Conflicting "virtual OVERRIDE" style checks

[Yuri Wiitala]

I recently wrote some code that added a virtual method override.  `git cl presubmit` complained with this message:

Running presubmit upload checks ...

/mnt/ssd/miu/chromium/src/chrome/browser/ui/views/tabs/tab.cc:240:  "virtual" is redundant since function is already declared as "override"  [readability/inheritance] [4]

So, I did as it asked; and then Clang had something to say:

../../chrome/browser/ui/views/tabs/tab.cc:240:3: error: [chromium-style] Overriding method must have "virtual" keyword.

  views::View* GetTooltipHandlerForPoint(const gfx::Point& point) override {

What's up with that?  ;-)

[Alex Vakulenko]

It is weird. But google style guide explicitly states this: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html?showone=The__define_Guard#Inheritance

    For clarity, use exactly one of override, final, or virtual when declaring an override

So it seems like clang's rule is wrong.

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

[Avi Drissman]

Someone must have updated the presubmit recently. We're in a bit of a transition period while we move from OVERRIDE/FINAL to override/final. They're definitely going to be updating the clang plugin to match.

[Daniel Cheng]

Please ignore the presubmit for now. I'll be reverting the presubmit check shortly.

The code for the clang plugin has already been updated, but a new version of clang with the changes has not yet been built and pushed.

Daniel

[Daniel Cheng]

OK, so it wasn't a recent presubmit change that triggered this. cpplint.py only knows how to look for override, not OVERRIDE, so it was ignoring it before. I've added a temporary suppression for this cpplint warning in https://codereview.chromium.org/627163002/. It works for me locally; let me know if that fixes it for you.

Daniel

[Alex Vakulenko]

By the way, I'd suggest re-evaluating current cpplint suppression filters on the chromium code. cpplint has been updated in the past to handle tricky situations better. E.g. it now understands #includes inside #ifdef blocks so it doesn't complain about multiple header file inclusions and include order better (the currently suppressed build/include_order). Also a lot of false positives with "readability/casting" and function pointers have been fixed.

We currently can run most of Chrome OS code through the linter with all the checks turned on and only in a few cases we had to add // NOLINT (or the new // NOLINTNEXTLINE) to suppress the warnings...

[Alex Vakulenko]

Another thing I forgot to mention is that cpplint now supports per-directory configuration. So you can override default warning filters for a particular directory and its sub-directories.

You need to just add CPPLINT.cfg in a directory and specify:

filters=+build/include,+readability/casting

and then you can clean up those warnings in a smaller portion of the code. Once everything is cleaned up, you can delete those .cfg files and make the global filter changes in your presubmit_canned_checks.py.

Right now the disabled warnings seem to leave too much room for diverging from the style guide and there seems to be no timeline as to when these issues are resolved:

• build/include : Too many; fix in the future.
  
• build/include_order : Not happening; #ifdefed includes.
  
• build/namespace : I'm surprised by how often we violate this rule.
  
• readability/casting : Mistakes a whole bunch of function pointer.
• runtime/int : Can be fixed long term; volume of errors too high
• runtime/virtual : Broken now, but can be fixed in the future?
  
• readability/inheritance : Temporary, while the OVERRIDE and FINAL fixup is in progress
  

 

[Daniel Cheng]

readability/inheritance seems pretty clear to me =)

Daniel

[James Cook]

So which do we prefer?

1. virtual void Foo() override;

2. void Foo() override;

https://code.google.com/p/chromium/issues/detail?id=417463 has a large number of conversions to (1).

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html?showone=The__define_Guard#Inheritance implies (2).

James

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

[Avi Drissman]

Eventually we're going to do 2, as mandated by the style guide. But right now the Clang plugin insists on 1, so we need to fix that.

[Peter Kasting]

On Tue, Oct 7, 2014 at 9:34 AM, James Cook <jame...@chromium.org> wrote:

So which do we prefer?

1. virtual void Foo() override;

2. void Foo() override;

https://code.google.com/p/chromium/issues/detail?id=417463 has a large number of conversions to (1).

We're converting from where we are now to (1) instead of converting straight to (2) because converting to (1) can be done mechanically by a script, and then conversion from there to (2) can be done with the help of clang.  The end goal is (2).

PK

[Adrienne Walker]

2014-10-07 11:03 GMT-07:00 Peter Kasting <pkas...@chromium.org>:

> We're converting from where we are now to (1) instead of converting straight
> to (2) because converting to (1) can be done mechanically by a script, and
> then conversion from there to (2) can be done with the help of clang. The
> end goal is (2).

cc's presubmit has cpplint.py warnings turned way up to level 1, since
it was previously clean. All of the patches to convert to (1)
currently cause a boatload of warnings like:

cc/trees/single_thread_proxy.h:108: "virtual" is redundant since

function is already declared as "override" [readability/inheritance]
[4]

When is (2) going to happen?

[Daniel Cheng]

Sorry, I didn't realize that cc/ doesn't use the canned presubmit check for cpplint.

Unfortunately, (2) cannot happen until a new clang binary with my changes is built. Once clang rolls, the cleanup will largely be automated and should take <1 day*.

* Windows-only code may take a little longer.