plugin: Do not warn on virtual/override stuff coming from macros in system headers.

plugin: Do not warn on virtual/override stuff coming from macros in system headers.

END_COM_MAP() adds a `virtual long AddRef() = 0` to Chromium classes, and
we don't want to warn on that.

(This also means that if there was a system header defining something like
FINAL, we'd no longer warn on that which is a bit unfortunate -- but it's
consistent with how the other plugin warnings work.)

Also add a comment that makes the windows.h in a test look less wtf-y. This
requires updating a golden file.

BUG=483128
Committed: https://crrev.com/3961d110959aaf9655af68dfaf041cd5f0f991d4
Cr-Commit-Position: refs/heads/master@{#328665}

[Nico, 2015-05-06 22:41:06]

thakis@chromium.org changed reviewers:
+ dcheng@chromium.org, hans@chromium.org

[Nico, 2015-05-06 22:41:06]

First to review wins!

I looked at other places where we emit diags, and I think the other places don't
need this.

[Nico, 2015-05-06 22:41:59]

(patch set 2 has the actual plugin change too. I had reverted it locally to
check that the test fails without it; apologies)

On Wed, May 6, 2015 at 3:41 PM, <thakis@chromium.org> wrote:

> Reviewers: hans, dcheng,
>
> Message:
> First to review wins!
>
> I looked at other places where we emit diags, and I think the other places
> don't
> need this.
>
> Description:
> plugin: Do not warn on virtual/override stuff coming from macros in system
> headers.
>
> END_COM_MAP() adds a `virtual long AddRef() = 0` to Chromium classes, and
> we don't want to warn on that.
>
> (This also means that if there was a system header defining something like
> FINAL, we'd no longer warn on that which is a bit unfortunate -- but it's
> consistent with how the other plugin warnings work.)
>
> Also add a comment that makes the windows.h in a test look less wtf-y. This
> requires updating a golden file.
>
> BUG=483128
>
> Please review this at https://codereview.chromium.org/1129093002/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+40, -21 lines):
>   M tools/clang/plugins/tests/system/windows.h
>   M tools/clang/plugins/tests/virtual_specifiers.cpp
>   M tools/clang/plugins/tests/virtual_specifiers.txt
>
>
> Index: tools/clang/plugins/tests/system/windows.h
> diff --git a/tools/clang/plugins/tests/system/windows.h
> b/tools/clang/plugins/tests/system/windows.h
> index
>
f93d5e2db9f510fd16d389396b42a4818a4b418e..66e923de45d5b6bfd6ba9b20728ec8ec6b83349d
> 100644
> --- a/tools/clang/plugins/tests/system/windows.h
> +++ b/tools/clang/plugins/tests/system/windows.h
> @@ -7,4 +7,9 @@
>
>  #define STDMETHOD(x) virtual void x
>
> +#define END_COM_MAP() virtual void AddRef() = 0;
> +
> +#define SYSTEM_REDUNDANT1 virtual void NonVirtualFinal() final
> +#define SYSTEM_REDUNDANT2 virtual void Virtual() override final
> +
>  #endif  // TOOLS_CLANG_PLUGINS_TESTS_SYSTEM_WINDOWS_H_
> Index: tools/clang/plugins/tests/virtual_specifiers.cpp
> diff --git a/tools/clang/plugins/tests/virtual_specifiers.cpp
> b/tools/clang/plugins/tests/virtual_specifiers.cpp
> index
>
210324809f1ee7d7a41ad3f3108a61f52d6ffd9f..f566ac4f2a6cd205c493f8446803f7f397d29ab2
> 100644
> --- a/tools/clang/plugins/tests/virtual_specifiers.cpp
> +++ b/tools/clang/plugins/tests/virtual_specifiers.cpp
> @@ -5,6 +5,7 @@
>  // Tests for chromium style checks for virtual/override/final specifiers
> on
>  // virtual methods.
>
> +// Note: This is not actual windows.h but the stub file in
> system/windows.h
>  #include <windows.h>
>
>  // Purposely use macros to test that the FixIt hints don't try to remove
> the
> @@ -116,3 +117,16 @@ class MyNotTest : public testing::NotTest {
>    virtual ~MyNotTest();
>    virtual void SetUp() override;
>  };
> +
> +class MacroBase {
> + public:
> +  virtual void AddRef() = 0;
> +  virtual void Virtual() {}
> +};
> +
> +class Sub : public MacroBase {
> +  // Shouldn't warn.
> +  END_COM_MAP()
> +  SYSTEM_REDUNDANT1;
> +  SYSTEM_REDUNDANT2;
> +};
> Index: tools/clang/plugins/tests/virtual_specifiers.txt
> diff --git a/tools/clang/plugins/tests/virtual_specifiers.txt
> b/tools/clang/plugins/tests/virtual_specifiers.txt
> index
>
135ebdd32373a4b9ba924319dfabade9092ca7be..7b36d2330d5920f17cbfbe65f882f65acd099b3a
> 100644
> --- a/tools/clang/plugins/tests/virtual_specifiers.txt
> +++ b/tools/clang/plugins/tests/virtual_specifiers.txt
> @@ -1,69 +1,69 @@
> -virtual_specifiers.cpp:38:22: warning: [chromium-style] Overriding method
> must be marked with 'override' or 'final'.
> +virtual_specifiers.cpp:39:22: warning: [chromium-style] Overriding method
> must be marked with 'override' or 'final'.
>    ~MissingOverride() {}
>                       ^
>                        override
> -virtual_specifiers.cpp:39:12: warning: [chromium-style] Overriding method
> must be marked with 'override' or 'final'.
> +virtual_specifiers.cpp:40:12: warning: [chromium-style] Overriding method
> must be marked with 'override' or 'final'.
>    void F() {}
>             ^
>              override
> -virtual_specifiers.cpp:45:3: warning: [chromium-style] 'virtual' is
> redundant; 'override' implies 'virtual'.
> +virtual_specifiers.cpp:46:3: warning: [chromium-style] 'virtual' is
> redundant; 'override' implies 'virtual'.
>    virtual ~VirtualAndOverride() OVERRIDE {}
>    ^~~~~~~~
> -virtual_specifiers.cpp:46:3: warning: [chromium-style] 'virtual' is
> redundant; 'override' implies 'virtual'.
> +virtual_specifiers.cpp:47:3: warning: [chromium-style] 'virtual' is
> redundant; 'override' implies 'virtual'.
>    virtual void F() OVERRIDE {}
>    ^~~~~~~~
> -virtual_specifiers.cpp:51:3: warning: [chromium-style] 'virtual' is
> redundant; 'final' implies 'virtual'.
> +virtual_specifiers.cpp:52:3: warning: [chromium-style] 'virtual' is
> redundant; 'final' implies 'virtual'.
>    virtual ~VirtualAndFinal() FINAL {}
>    ^~~~~~~~
> -virtual_specifiers.cpp:52:3: warning: [chromium-style] 'virtual' is
> redundant; 'final' implies 'virtual'.
> +virtual_specifiers.cpp:53:3: warning: [chromium-style] 'virtual' is
> redundant; 'final' implies 'virtual'.
>    virtual void F() FINAL {}
>    ^~~~~~~~
> -virtual_specifiers.cpp:57:3: warning: [chromium-style] 'virtual' is
> redundant; 'override' implies 'virtual'.
> +virtual_specifiers.cpp:58:3: warning: [chromium-style] 'virtual' is
> redundant; 'override' implies 'virtual'.
>    virtual ~VirtualAndOverrideFinal() OVERRIDE FINAL {}
>    ^~~~~~~~
> -virtual_specifiers.cpp:57:38: warning: [chromium-style] 'override' is
> redundant; 'final' implies 'override'.
> +virtual_specifiers.cpp:58:38: warning: [chromium-style] 'override' is
> redundant; 'final' implies 'override'.
>    virtual ~VirtualAndOverrideFinal() OVERRIDE FINAL {}
>                                       ^~~~~~~~~
> -virtual_specifiers.cpp:12:18: note: expanded from macro 'OVERRIDE'
> +virtual_specifiers.cpp:13:18: note: expanded from macro 'OVERRIDE'
>  #define OVERRIDE override
>                   ^
> -virtual_specifiers.cpp:58:3: warning: [chromium-style] 'virtual' is
> redundant; 'override' implies 'virtual'.
> +virtual_specifiers.cpp:59:3: warning: [chromium-style] 'virtual' is
> redundant; 'override' implies 'virtual'.
>    virtual void F() OVERRIDE FINAL {}
>    ^~~~~~~~
> -virtual_specifiers.cpp:58:20: warning: [chromium-style] 'override' is
> redundant; 'final' implies 'override'.
> +virtual_specifiers.cpp:59:20: warning: [chromium-style] 'override' is
> redundant; 'final' implies 'override'.
>    virtual void F() OVERRIDE FINAL {}
>                     ^~~~~~~~~
> -virtual_specifiers.cpp:12:18: note: expanded from macro 'OVERRIDE'
> +virtual_specifiers.cpp:13:18: note: expanded from macro 'OVERRIDE'
>  #define OVERRIDE override
>                   ^
> -virtual_specifiers.cpp:63:23: warning: [chromium-style] 'override' is
> redundant; 'final' implies 'override'.
> +virtual_specifiers.cpp:64:23: warning: [chromium-style] 'override' is
> redundant; 'final' implies 'override'.
>    ~OverrideAndFinal() OVERRIDE FINAL {}
>                        ^~~~~~~~~
> -virtual_specifiers.cpp:12:18: note: expanded from macro 'OVERRIDE'
> +virtual_specifiers.cpp:13:18: note: expanded from macro 'OVERRIDE'
>  #define OVERRIDE override
>                   ^
> -virtual_specifiers.cpp:64:12: warning: [chromium-style] 'override' is
> redundant; 'final' implies 'override'.
> +virtual_specifiers.cpp:65:12: warning: [chromium-style] 'override' is
> redundant; 'final' implies 'override'.
>    void F() OVERRIDE FINAL {}
>             ^~~~~~~~~
> -virtual_specifiers.cpp:12:18: note: expanded from macro 'OVERRIDE'
> +virtual_specifiers.cpp:13:18: note: expanded from macro 'OVERRIDE'
>  #define OVERRIDE override
>                   ^
> -virtual_specifiers.cpp:69:20: warning: [chromium-style] Overriding method
> must be marked with 'override' or 'final'.
> +virtual_specifiers.cpp:70:20: warning: [chromium-style] Overriding method
> must be marked with 'override' or 'final'.
>    virtual void F() = 0;
>                     ^
>                      override
> -virtual_specifiers.cpp:73:12: warning: [chromium-style] Overriding method
> must be marked with 'override' or 'final'.
> +virtual_specifiers.cpp:74:12: warning: [chromium-style] Overriding method
> must be marked with 'override' or 'final'.
>    void F() = 0;
>             ^
>              override
> -virtual_specifiers.cpp:81:3: warning: [chromium-style] 'virtual' is
> redundant; 'override' implies 'virtual'.
> +virtual_specifiers.cpp:82:3: warning: [chromium-style] 'virtual' is
> redundant; 'override' implies 'virtual'.
>    virtual void F() override = 0;
>    ^~~~~~~~
> -virtual_specifiers.cpp:110:20: warning: [chromium-style] Overriding
> method must be marked with 'override' or 'final'.
> +virtual_specifiers.cpp:111:20: warning: [chromium-style] Overriding
> method must be marked with 'override' or 'final'.
>    virtual ~MyTest();
>                     ^
>                      override
> -virtual_specifiers.cpp:111:3: warning: [chromium-style] 'virtual' is
> redundant; 'override' implies 'virtual'.
> +virtual_specifiers.cpp:112:3: warning: [chromium-style] 'virtual' is
> redundant; 'override' implies 'virtual'.
>    virtual void SetUp() override;
>    ^~~~~~~~
>  17 warnings generated.
>
>
>

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

[dcheng, 2015-05-06 22:44:26]

https://codereview.chromium.org/1129093002/diff/20001/tools/clang/plugins/Fin...
File tools/clang/plugins/FindBadConstructsConsumer.cpp (right):

https://codereview.chromium.org/1129093002/diff/20001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:477: if
(!InBannedDirectory(manager.getSpellingLoc(loc))) {
It almost feels like this should be a helper.

reportIfLegit(loc, diagnostic_) << FixItHint::CreateRandom();

[hans, 2015-05-06 22:47:42]

+1 on dcheng's comment

https://codereview.chromium.org/1129093002/diff/20001/tools/clang/plugins/Fin...
File tools/clang/plugins/FindBadConstructsConsumer.cpp (right):

https://codereview.chromium.org/1129093002/diff/20001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:474: // Again, only emit the
warning if it doesn't originate from a macro in
Silly question: what does "Again" refer to here?

[Nico, 2015-05-06 22:49:24]

https://codereview.chromium.org/1129093002/diff/20001/tools/clang/plugins/Fin...
File tools/clang/plugins/FindBadConstructsConsumer.cpp (right):

https://codereview.chromium.org/1129093002/diff/20001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:420: // source range.
to this

https://codereview.chromium.org/1129093002/diff/20001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:477: if
(!InBannedDirectory(manager.getSpellingLoc(loc))) {
On 2015/05/06 22:44:26, dcheng wrote:
> It almost feels like this should be a helper.
> 
> reportIfLegit(loc, diagnostic_) << FixItHint::CreateRandom();

I considered that, but it's really only this function that needs it from what I
can tell. We already only diagnose records that are not from system headers, and
this is the only diag so far where stuff is added to non-system-header structs
with system-header macros.

[Nico, 2015-05-06 23:09:16]

(Also creating a dummy stream for fixits if nothing gets emitted is
annoying)
On May 6, 2015 3:49 PM, <thakis@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1129093002/diff/20001/tools/clang/plugins/Fin...
> File tools/clang/plugins/FindBadConstructsConsumer.cpp (right):
>
>
>
https://codereview.chromium.org/1129093002/diff/20001/tools/clang/plugins/Fin...
> tools/clang/plugins/FindBadConstructsConsumer.cpp:420: // source range.
> to this
>
>
>
https://codereview.chromium.org/1129093002/diff/20001/tools/clang/plugins/Fin...
> tools/clang/plugins/FindBadConstructsConsumer.cpp:477: if
> (!InBannedDirectory(manager.getSpellingLoc(loc))) {
> On 2015/05/06 22:44:26, dcheng wrote:
>
>> It almost feels like this should be a helper.
>>
>
>  reportIfLegit(loc, diagnostic_) << FixItHint::CreateRandom();
>>
>
> I considered that, but it's really only this function that needs it from
> what I can tell. We already only diagnose records that are not from
> system headers, and this is the only diag so far where stuff is added to
> non-system-header structs with system-header macros.
>
> https://codereview.chromium.org/1129093002/
>

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

[hans, 2015-05-06 23:13:45]

On 2015/05/06 22:49:24, Nico wrote:
> On 2015/05/06 22:44:26, dcheng wrote:
> > It almost feels like this should be a helper.
> > 
> > reportIfLegit(loc, diagnostic_) << FixItHint::CreateRandom();
> 
> I considered that, but it's really only this function that needs it from what
I
> can tell. We already only diagnose records that are not from system headers,
and
> this is the only diag so far where stuff is added to non-system-header structs
> with system-header macros.

ok, lgtm

[Nico, 2015-05-07 00:09:52]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2015-05-07 00:11:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129093002/20001

[commit-bot: I haz the power, 2015-05-07 00:38:42]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-07 00:39:42]

Patchset 2 (id:??) landed as
https://crrev.com/3961d110959aaf9655af68dfaf041cd5f0f991d4
Cr-Commit-Position: refs/heads/master@{#328665}