Update plugin to handle new style rules for virtual annotations.

Update plugin to handle new style rules for virtual specifiers.

Implements several new checks in the plugin, gated behind a flag:
- Only one of {virtual,override,final} should be ever used.
- Destructors must also be annotated correctly.
- A virtual final method that doesn't override anything should be
  devirtualized.

The test harness for the Chrome plugin has also been updated to stop
it from littering the source tree with object files, and to make the
golden files easier to update.

BUG=417463
Committed: https://crrev.com/993e04f76665c46336a6c790706d863adf64b49c
Cr-Commit-Position: refs/heads/master@{#297129}

[dcheng, 2014-09-24 07:11:44]

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

[dcheng, 2014-09-24 07:29:03]

dcheng@chromium.org changed reviewers:
+ ajwong@google.com

[dcheng, 2014-09-24 16:31:02]

I've updated the plugin to implement the proposed style guideline changes in the
C++11 feature proposal thread for override/final. I've also added a few extra
checks that we didn't previously have, but I've guarded those behind the
strict-virtual-annotations flag just in case there's code unexpectedly doing
that.

If the proposed feature becomes allowed, I would see the migration occurring in
several steps:
1) Giant search and replace for OVERRIDE/FINAL to replace them with the
standard, lowercase forms.
2) Run a build to automatically apply fixit hints (caveat: this seems to be
harder than it used to be. I thought clang used to have a flag to do this
directly, but I think the functionality is in clang-check now?)
3) Enable strict virtual annotation checking.

[Nico, 2014-09-24 16:33:42]

On 2014/09/24 16:31:02, dcheng wrote:
> I've updated the plugin to implement the proposed style guideline changes in
the
> C++11 feature proposal thread for override/final. I've also added a few extra
> checks that we didn't previously have, but I've guarded those behind the
> strict-virtual-annotations flag just in case there's code unexpectedly doing
> that.
> 
> If the proposed feature becomes allowed, I would see the migration occurring
in
> several steps:
> 1) Giant search and replace for OVERRIDE/FINAL to replace them with the
> standard, lowercase forms.
> 2) Run a build to automatically apply fixit hints (caveat: this seems to be
> harder than it used to be. I thought clang used to have a flag to do this
> directly, but I think the functionality is in clang-check now?

-Xclang -fixit should do it (`clang -cc1 --help 2>&1 | grep fixi`)

> 3) Enable strict virtual annotation checking.

[Nico, 2014-09-24 16:37:01]

thakis@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Nico, 2014-09-24 16:37:02]

The plan sounds good to me. Maybe rsleevi can look at this, since he worked on
the existing virtual function stuff.

I'll wait until the thread is 24h old so that all timezones had a chance to
chime in, but at the moment it looks like everyone's supportive of both using
lowercase override/final and of mass-find-replacing everyting.

[dcheng, 2014-09-24 17:12:40]

Adding rsleevi per your suggestion.

[Nico, 2014-09-24 17:38:07]

https://codereview.chromium.org/597863002/diff/80001/tools/clang/plugins/Find...
File tools/clang/plugins/FindBadConstructsConsumer.cpp (right):

https://codereview.chromium.org/597863002/diff/80001/tools/clang/plugins/Find...
tools/clang/plugins/FindBadConstructsConsumer.cpp:30: "final; "
Once the strict checks are on, we won't need this, right? If only "virtual" or
"final" are allowed, then clang will error if you say "final" on a method that's
not virtual.

[dcheng, 2014-09-24 18:53:12]

On 2014/09/24 at 16:33:42, thakis wrote:
> On 2014/09/24 16:31:02, dcheng wrote:
> > I've updated the plugin to implement the proposed style guideline changes in
the
> > C++11 feature proposal thread for override/final. I've also added a few
extra
> > checks that we didn't previously have, but I've guarded those behind the
> > strict-virtual-annotations flag just in case there's code unexpectedly doing
> > that.
> > 
> > If the proposed feature becomes allowed, I would see the migration occurring
in
> > several steps:
> > 1) Giant search and replace for OVERRIDE/FINAL to replace them with the
> > standard, lowercase forms.
> > 2) Run a build to automatically apply fixit hints (caveat: this seems to be
> > harder than it used to be. I thought clang used to have a flag to do this
> > directly, but I think the functionality is in clang-check now?
> 
> -Xclang -fixit should do it (`clang -cc1 --help 2>&1 | grep fixi`)
> 
> > 3) Enable strict virtual annotation checking.

So I tried this in combination with strict virtual annotations. It quickly
exploded inside NaCl:
../../native_client/src/trusted/validator_x86/nccopycode_stores.S:62:1: error:
expected identifier or '('
DEFINE_GLOBAL_HIDDEN_FUNCTION(onestore_memmove4):
^
../../native_client/src/include/nacl_asm.h:64:3: note: expanded from macro
'DEFINE_GLOBAL_HIDDEN_FUNCTION'
  .globl IDENTIFIER(n); HIDDEN(n); IDENTIFIER(n)
  ^
../../native_client/src/trusted/validator_x86/nccopycode_stores.S:62:1: note:
FIX-IT detected an error it cannot fix
../../native_client/src/include/nacl_asm.h:64:3: note: expanded from macro
'DEFINE_GLOBAL_HIDDEN_FUNCTION'
  .globl IDENTIFIER(n); HIDDEN(n); IDENTIFIER(n)
  ^

After disable_nacl=1, the build still runs into mysterious failures after the
fixit rewriter rewrites a couple things:
ninja: Entering directory `out/Debug'
[1/16734] CXX obj/third_party/snappy/src/snappy.snappy-sinksource.o
/usr/local/google/ssd/src/chrome/src/third_party/binutils/Linux_x64/Release/bin/objcopy:
'obj/third_party/snappy/src/snappy.snappy-sinksource.o': No such file

So unfortunately this isn't quite as simple as I hoped.

[dcheng, 2014-09-24 20:05:42]

On 2014/09/24 at 18:53:12, dcheng wrote:
> On 2014/09/24 at 16:33:42, thakis wrote:
> > On 2014/09/24 16:31:02, dcheng wrote:
> > > I've updated the plugin to implement the proposed style guideline changes
in the
> > > C++11 feature proposal thread for override/final. I've also added a few
extra
> > > checks that we didn't previously have, but I've guarded those behind the
> > > strict-virtual-annotations flag just in case there's code unexpectedly
doing
> > > that.
> > > 
> > > If the proposed feature becomes allowed, I would see the migration
occurring in
> > > several steps:
> > > 1) Giant search and replace for OVERRIDE/FINAL to replace them with the
> > > standard, lowercase forms.
> > > 2) Run a build to automatically apply fixit hints (caveat: this seems to
be
> > > harder than it used to be. I thought clang used to have a flag to do this
> > > directly, but I think the functionality is in clang-check now?
> > 
> > -Xclang -fixit should do it (`clang -cc1 --help 2>&1 | grep fixi`)
> > 
> > > 3) Enable strict virtual annotation checking.
> 
> So I tried this in combination with strict virtual annotations. It quickly
exploded inside NaCl:
> ../../native_client/src/trusted/validator_x86/nccopycode_stores.S:62:1: error:
expected identifier or '('
> DEFINE_GLOBAL_HIDDEN_FUNCTION(onestore_memmove4):
> ^
> ../../native_client/src/include/nacl_asm.h:64:3: note: expanded from macro
'DEFINE_GLOBAL_HIDDEN_FUNCTION'
>   .globl IDENTIFIER(n); HIDDEN(n); IDENTIFIER(n)
>   ^
> ../../native_client/src/trusted/validator_x86/nccopycode_stores.S:62:1: note:
FIX-IT detected an error it cannot fix
> ../../native_client/src/include/nacl_asm.h:64:3: note: expanded from macro
'DEFINE_GLOBAL_HIDDEN_FUNCTION'
>   .globl IDENTIFIER(n); HIDDEN(n); IDENTIFIER(n)
>   ^
> 
> After disable_nacl=1, the build still runs into mysterious failures after the
fixit rewriter rewrites a couple things:
> ninja: Entering directory `out/Debug'
> [1/16734] CXX obj/third_party/snappy/src/snappy.snappy-sinksource.o
>
/usr/local/google/ssd/src/chrome/src/third_party/binutils/Linux_x64/Release/bin/objcopy:
'obj/third_party/snappy/src/snappy.snappy-sinksource.o': No such file
> 
> So unfortunately this isn't quite as simple as I hoped.

(Actually, thinking about this more, I bet that clang isn't generating object
files in fixit mode.)

[Nico, 2014-09-24 20:12:03]

There's -fixit-recompile, maybe that does The Thing?

On Wed, Sep 24, 2014 at 1:05 PM, <dcheng@chromium.org> wrote:

> On 2014/09/24 at 18:53:12, dcheng wrote:
>
>> On 2014/09/24 at 16:33:42, thakis wrote:
>> > On 2014/09/24 16:31:02, dcheng wrote:
>> > > I've updated the plugin to implement the proposed style guideline
>> changes
>>
> in the
>
>> > > C++11 feature proposal thread for override/final. I've also added a
>> few
>>
> extra
>
>> > > checks that we didn't previously have, but I've guarded those behind
>> the
>> > > strict-virtual-annotations flag just in case there's code unexpectedly
>>
> doing
>
>> > > that.
>> > >
>> > > If the proposed feature becomes allowed, I would see the migration
>>
> occurring in
>
>> > > several steps:
>> > > 1) Giant search and replace for OVERRIDE/FINAL to replace them with
>> the
>> > > standard, lowercase forms.
>> > > 2) Run a build to automatically apply fixit hints (caveat: this seems
>> to
>>
> be
>
>> > > harder than it used to be. I thought clang used to have a flag to do
>> this
>> > > directly, but I think the functionality is in clang-check now?
>> >
>> > -Xclang -fixit should do it (`clang -cc1 --help 2>&1 | grep fixi`)
>> >
>> > > 3) Enable strict virtual annotation checking.
>>
>
>  So I tried this in combination with strict virtual annotations. It quickly
>>
> exploded inside NaCl:
>
>> ../../native_client/src/trusted/validator_x86/nccopycode_stores.S:62:1:
>> error:
>>
> expected identifier or '('
>
>> DEFINE_GLOBAL_HIDDEN_FUNCTION(onestore_memmove4):
>> ^
>> ../../native_client/src/include/nacl_asm.h:64:3: note: expanded from
>> macro
>>
> 'DEFINE_GLOBAL_HIDDEN_FUNCTION'
>
>>    .globl IDENTIFIER(n); HIDDEN(n); IDENTIFIER(n)
>>    ^
>> ../../native_client/src/trusted/validator_x86/nccopycode_stores.S:62:1:
>> note:
>>
> FIX-IT detected an error it cannot fix
>
>> ../../native_client/src/include/nacl_asm.h:64:3: note: expanded from
>> macro
>>
> 'DEFINE_GLOBAL_HIDDEN_FUNCTION'
>
>>    .globl IDENTIFIER(n); HIDDEN(n); IDENTIFIER(n)
>>    ^
>>
>
>  After disable_nacl=1, the build still runs into mysterious failures after
>> the
>>
> fixit rewriter rewrites a couple things:
>
>> ninja: Entering directory `out/Debug'
>> [1/16734] CXX obj/third_party/snappy/src/snappy.snappy-sinksource.o
>>
>
> /usr/local/google/ssd/src/chrome/src/third_party/
> binutils/Linux_x64/Release/bin/objcopy:
> 'obj/third_party/snappy/src/snappy.snappy-sinksource.o': No such file
>
>  So unfortunately this isn't quite as simple as I hoped.
>>
>
> (Actually, thinking about this more, I bet that clang isn't generating
> object
> files in fixit mode.)
>
> https://codereview.chromium.org/597863002/
>

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

[dcheng, 2014-09-24 20:12:11]

On 2014/09/24 at 20:05:42, dcheng wrote:
> On 2014/09/24 at 18:53:12, dcheng wrote:
> > On 2014/09/24 at 16:33:42, thakis wrote:
> > > On 2014/09/24 16:31:02, dcheng wrote:
> > > > I've updated the plugin to implement the proposed style guideline
changes in the
> > > > C++11 feature proposal thread for override/final. I've also added a few
extra
> > > > checks that we didn't previously have, but I've guarded those behind the
> > > > strict-virtual-annotations flag just in case there's code unexpectedly
doing
> > > > that.
> > > > 
> > > > If the proposed feature becomes allowed, I would see the migration
occurring in
> > > > several steps:
> > > > 1) Giant search and replace for OVERRIDE/FINAL to replace them with the
> > > > standard, lowercase forms.
> > > > 2) Run a build to automatically apply fixit hints (caveat: this seems to
be
> > > > harder than it used to be. I thought clang used to have a flag to do
this
> > > > directly, but I think the functionality is in clang-check now?
> > > 
> > > -Xclang -fixit should do it (`clang -cc1 --help 2>&1 | grep fixi`)
> > > 
> > > > 3) Enable strict virtual annotation checking.
> > 
> > So I tried this in combination with strict virtual annotations. It quickly
exploded inside NaCl:
> > ../../native_client/src/trusted/validator_x86/nccopycode_stores.S:62:1:
error: expected identifier or '('
> > DEFINE_GLOBAL_HIDDEN_FUNCTION(onestore_memmove4):
> > ^
> > ../../native_client/src/include/nacl_asm.h:64:3: note: expanded from macro
'DEFINE_GLOBAL_HIDDEN_FUNCTION'
> >   .globl IDENTIFIER(n); HIDDEN(n); IDENTIFIER(n)
> >   ^
> > ../../native_client/src/trusted/validator_x86/nccopycode_stores.S:62:1:
note: FIX-IT detected an error it cannot fix
> > ../../native_client/src/include/nacl_asm.h:64:3: note: expanded from macro
'DEFINE_GLOBAL_HIDDEN_FUNCTION'
> >   .globl IDENTIFIER(n); HIDDEN(n); IDENTIFIER(n)
> >   ^
> > 
> > After disable_nacl=1, the build still runs into mysterious failures after
the fixit rewriter rewrites a couple things:
> > ninja: Entering directory `out/Debug'
> > [1/16734] CXX obj/third_party/snappy/src/snappy.snappy-sinksource.o
> >
/usr/local/google/ssd/src/chrome/src/third_party/binutils/Linux_x64/Release/bin/objcopy:
'obj/third_party/snappy/src/snappy.snappy-sinksource.o': No such file
> > 
> > So unfortunately this isn't quite as simple as I hoped.
> 
> (Actually, thinking about this more, I bet that clang isn't generating object
files in fixit mode.)

Sorry for the spam. Apparently there's a clang flag for this too:
-fixit-recompile. There are still a bunch of warnings in third_party directories
getting triggered, but I'm cautiously optimistic that this approach will
actually work now.

[dcheng, 2014-09-25 00:12:54]

PTAL. I've streamlined the tests and updated the logic to make it apply the same
logic to destructors, to conform to the C++ style guide.

https://codereview.chromium.org/597863002/diff/80001/tools/clang/plugins/Find...
File tools/clang/plugins/FindBadConstructsConsumer.cpp (right):

https://codereview.chromium.org/597863002/diff/80001/tools/clang/plugins/Find...
tools/clang/plugins/FindBadConstructsConsumer.cpp:30: "final; "
On 2014/09/24 at 17:38:06, Nico (hiding) wrote:
> Once the strict checks are on, we won't need this, right? If only "virtual" or
"final" are allowed, then clang will error if you say "final" on a method that's
not virtual.

This is, strictly speaking, unnecessary. But I think it's nice to prompt people
to devirtualize something if possible, if you end up in this state through
refactorings.

[dcheng, 2014-09-25 00:13:54]

I forgot to add: I have a proof of concept patch to do the rewriting here:
https://codereview.chromium.org/598073004

I didn't finish running it across the entire codebase, because -j1 is extremely
slow, but it seemed to be doing approximately the right thing.

[Nico, 2014-09-25 21:19:35]

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

[Nico, 2014-09-25 21:19:36]

Hans, can you look at this maybe? I didn't get to it :-/

[hans, 2014-09-25 21:23:22]

On 2014/09/25 21:19:36, Nico (away until Oct 27) wrote:
> Hans, can you look at this maybe? I didn't get to it :-/

Will try to look later this afternoon.

[hans, 2014-09-26 01:29:29]

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/Fin...
File tools/clang/plugins/FindBadConstructsAction.cpp (right):

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsAction.cpp:39: } else if (args[0] ==
"strict-virtual-specifiers") {
args[i]?

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

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:19: "[chromium-style]
Overriding method must be marked with override or final.";
pretty nitty, and we probably haven't done this so far, but I think we should
put single quotes around 'override' and 'final here. That corresponds to how
Clang prints its diagnostics.

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:21: "[chromium-style] %0 is
redundant; %1 implies %0.";
as per above, I'd put single quotes around the %0's here. (it seems %1 already
gets single quotes, because we're passing in an Attr)

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:27: "method should be
devirtualized";
s/on a base class/in a base class/

I'm not sure it would be obvious to everyone what "should be devirtualized"
means here. I'm also not sure about the "base class" part.

Maybe: "[chromium-style] The virtual method does not override anything and is
final; it should be made non-virtual". (But could the fix sometimes be to make
it non-final?)

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:76: const CXXMethodDecl*
method) {
It might be overkill, but you could reach into the SourceManager and check the
bytes at the source range you're looking at here actually say "virtual", and
bail out if it doesn't.

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:344: // subclasses, only the
final specifier may be used.
"not meant to be further subclasses" sounds weird.

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:406: <<
FixItHint::CreateRemoval(GuessSourceRangeForVirtual(manager, method))
Removing the virtual specifier should probably be gated on has_virtual.

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/tes...
File tools/clang/plugins/tests/test.sh (right):

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/tes...
tools/clang/plugins/tests/test.sh:36: local output="$("${CLANG_PATH}"
-fsyntax-only -c -Wno-c++11-extensions \
can probably drop the -c now that we pass -fsyntax-only

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/tes...
tools/clang/plugins/tests/test.sh:47: cat > ${2}-actual << EOF
Won't this leave a file in the source tree? It seems handy for testing, though.

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/tes...
File tools/clang/plugins/tests/virtual_dtor_specifiers.cpp (right):

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/tes...
tools/clang/plugins/tests/virtual_dtor_specifiers.cpp:1: // Simple sanity test
that virtual dtors are also being checked.
I would probably have put this in the same file as the test for the virtual
methods.

[dcheng, 2014-09-26 07:07:14]

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/Fin...
File tools/clang/plugins/FindBadConstructsAction.cpp (right):

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsAction.cpp:39: } else if (args[0] ==
"strict-virtual-specifiers") {
On 2014/09/26 01:29:29, hans wrote:
> args[i]?

Done.

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

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:19: "[chromium-style]
Overriding method must be marked with override or final.";
On 2014/09/26 01:29:29, hans wrote:
> pretty nitty, and we probably haven't done this so far, but I think we should
> put single quotes around 'override' and 'final here. That corresponds to how
> Clang prints its diagnostics.

Done.

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:21: "[chromium-style] %0 is
redundant; %1 implies %0.";
On 2014/09/26 01:29:29, hans wrote:
> as per above, I'd put single quotes around the %0's here. (it seems %1 already
> gets single quotes, because we're passing in an Attr)

Done, though I opted to just fix the instance where we pass in a string literal
rather than adding quotes here.

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:27: "method should be
devirtualized";
On 2014/09/26 01:29:29, hans wrote:
> s/on a base class/in a base class/
> 
> I'm not sure it would be obvious to everyone what "should be devirtualized"
> means here. I'm also not sure about the "base class" part.
> 
> Maybe: "[chromium-style] The virtual method does not override anything and is
> final; it should be made non-virtual". (But could the fix sometimes be to make
> it non-final?)

Updated with your text and weakened the suggestion a bit. I think it's just
worth prodding people to make something non-virtual (they might think about if
it really still needs to be virtual).

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:76: const CXXMethodDecl*
method) {
On 2014/09/26 01:29:29, hans wrote:
> It might be overkill, but you could reach into the SourceManager and check the
> bytes at the source range you're looking at here actually say "virtual", and
> bail out if it doesn't.

Heh. Done.

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:344: // subclasses, only the
final specifier may be used.
On 2014/09/26 at 01:29:29, hans wrote:
> "not meant to be further subclasses" sounds weird.

Oops, that's a typo. Fixed.

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:406: <<
FixItHint::CreateRemoval(GuessSourceRangeForVirtual(manager, method))
On 2014/09/26 01:29:29, hans wrote:
> Removing the virtual specifier should probably be gated on has_virtual.

It's not needed, because the following does not compile:
class Base {
 public:
  void F() final;
};
will trigger a warning that a non-virtual method cannot be final.

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/tes...
File tools/clang/plugins/tests/test.sh (right):

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/tes...
tools/clang/plugins/tests/test.sh:36: local output="$("${CLANG_PATH}"
-fsyntax-only -c -Wno-c++11-extensions \
On 2014/09/26 01:29:29, hans wrote:
> can probably drop the -c now that we pass -fsyntax-only

Done.

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/tes...
tools/clang/plugins/tests/test.sh:47: cat > ${2}-actual << EOF
On 2014/09/26 01:29:29, hans wrote:
> Won't this leave a file in the source tree? It seems handy for testing,
though.

Only if there's a diff. It makes it much easier to rebase things.

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/tes...
File tools/clang/plugins/tests/virtual_dtor_specifiers.cpp (right):

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/tes...
tools/clang/plugins/tests/virtual_dtor_specifiers.cpp:1: // Simple sanity test
that virtual dtors are also being checked.
On 2014/09/26 01:29:29, hans wrote:
> I would probably have put this in the same file as the test for the virtual
> methods.

Done.

[hans, 2014-09-27 00:36:10]

lgtm

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/tes...
File tools/clang/plugins/tests/test.sh (right):

https://codereview.chromium.org/597863002/diff/160001/tools/clang/plugins/tes...
tools/clang/plugins/tests/test.sh:47: cat > ${2}-actual << EOF
On 2014/09/26 07:07:14, dcheng wrote:
> On 2014/09/26 01:29:29, hans wrote:
> > Won't this leave a file in the source tree? It seems handy for testing,
> though.
> 
> Only if there's a diff. It makes it much easier to rebase things.

Oh, right. That makes sense.

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

https://codereview.chromium.org/597863002/diff/200001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:87: if
(!text.startswith("virtual"))
Any reason not to check if (text != "virtual") ?

https://codereview.chromium.org/597863002/diff/200001/tools/clang/plugins/tes...
File tools/clang/plugins/tests/virtual_base_method_also_final.cpp (right):

https://codereview.chromium.org/597863002/diff/200001/tools/clang/plugins/tes...
tools/clang/plugins/tests/virtual_base_method_also_final.cpp:7: // to be
emitted.
Nice!

[dcheng, 2014-09-28 18:35:39]

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

https://codereview.chromium.org/597863002/diff/200001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:87: if
(!text.startswith("virtual"))
On 2014/09/27 at 00:36:10, hans wrote:
> Any reason not to check if (text != "virtual") ?

I was worried about whitespace, but I forgot about StringRef::trim. However,
while investigating this a bit more, I found out I had a logic error. For
macros, the SourceRange I was passing to getSourceText() was returning the text
from the beginning of the macro definition to the end of where the macro was
expanded.

I changed this to just use token SourceRanges instead of char SourceRanges. I
thought this would break with macros that expanded to more than just virtual
(see the definition of method H() in virtual_base_method_also_final.cpp), but
Clang somehow knows to filter out that fixit hint.

[dcheng, 2014-09-28 18:35:46]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-09-28 18:36:10]

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

[commit-bot: I haz the power, 2014-09-28 18:42:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-28 18:42:04]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dcheng, 2014-09-28 18:48:19]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-09-28 18:49:12]

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

[commit-bot: I haz the power, 2014-09-28 19:28:03]

Committed patchset #14 (id:260001) as c9901d2f15e8d92011a176cd24544dfaa0a18459

[commit-bot: I haz the power, 2014-09-28 19:28:44]

Patchset 14 (id:??) landed as
https://crrev.com/993e04f76665c46336a6c790706d863adf64b49c
Cr-Commit-Position: refs/heads/master@{#297129}

[twasmundt36, 2016-08-02 04:12:06]

twasmundt36@gmail.com changed reviewers:
+ twasmundt36@gmail.com

[twasmundt36, 2016-08-02 04:12:47]

lgtm