clang-plugin: Enable RecursiveASTVisitor approach by default.

clang-plugin: Enable RecursiveASTVisitor approach by default.

This patch removes the ASTConsumer approach for checking TagDecls in
favor of always using RecursiveASTVisitor. This means that some of
the warnings have to be temporarily suppressed, since the ASTConsumer
approach was never checking them.

R=dcheng, thakis@chromium.org
BUG=436357
Committed: https://crrev.com/3d84fcc69061aac5c82ade4844749a4754ee5190
Cr-Commit-Position: refs/heads/master@{#375995}

[Nico, 2016-02-16 22:54:04]

plan generally sounds good to me, thanks! i'll leave the actual review to dcheng
(sounds like you're aware of how the plugin's bundled with clang and must not
become more strict in a roll, unless an explicit flag opts in to the strictness
-- else clang rolls become more difficult)

[vmpstr, 2016-02-16 22:58:23]

On 2016/02/16 22:54:04, Nico wrote:
> plan generally sounds good to me, thanks! i'll leave the actual review to
dcheng
> (sounds like you're aware of how the plugin's bundled with clang and must not
> become more strict in a roll, unless an explicit flag opts in to the
strictness
> -- else clang rolls become more difficult)

Yep, dcheng@ informed of this :) I'm doing some more builds to make sure that
Chromium builds clean with this patch.

[dcheng, 2016-02-16 23:03:50]

https://codereview.chromium.org/1703713002/diff/1/tools/clang/plugins/ChromeC...
File tools/clang/plugins/ChromeClassTester.h (right):

https://codereview.chromium.org/1703713002/diff/1/tools/clang/plugins/ChromeC...
tools/clang/plugins/ChromeClassTester.h:18: // TODO(vmpstr): Remove the
clang::ASTConsumer base and fold this class into
Can we just remove the base class now?

https://codereview.chromium.org/1703713002/diff/1/tools/clang/plugins/FindBad...
File tools/clang/plugins/FindBadConstructsConsumer.cpp (right):

https://codereview.chromium.org/1703713002/diff/1/tools/clang/plugins/FindBad...
tools/clang/plugins/FindBadConstructsConsumer.cpp:282: } else if
(options_.check_implicit_copy_ctors) {
Since this flag is about implicit constructors, and this else branch seems to
handle lots of things, would it make sense to check this flag on line 293
instead?

Also, do the plugin tests need to be updated? I know that we hardcode
with-ast-visitor for the tests right now: should we update this flag?

[vmpstr, 2016-02-16 23:13:27]

https://codereview.chromium.org/1703713002/diff/1/tools/clang/plugins/ChromeC...
File tools/clang/plugins/ChromeClassTester.h (right):

https://codereview.chromium.org/1703713002/diff/1/tools/clang/plugins/ChromeC...
tools/clang/plugins/ChromeClassTester.h:18: // TODO(vmpstr): Remove the
clang::ASTConsumer base and fold this class into
On 2016/02/16 23:03:50, dcheng wrote:
> Can we just remove the base class now?

Yep! Done.

https://codereview.chromium.org/1703713002/diff/1/tools/clang/plugins/FindBad...
File tools/clang/plugins/FindBadConstructsConsumer.cpp (right):

https://codereview.chromium.org/1703713002/diff/1/tools/clang/plugins/FindBad...
tools/clang/plugins/FindBadConstructsConsumer.cpp:282: } else if
(options_.check_implicit_copy_ctors) {
On 2016/02/16 23:03:50, dcheng wrote:
> Since this flag is about implicit constructors, and this else branch seems to
> handle lots of things, would it make sense to check this flag on line 293
> instead?

The move ctor branch would have the same issues as the copy ctor branch (I
couldn't think of a better flag name though)

The branch on 319 also generated additional errors with the RAV approach. Let me
dig in a bit more to see what those were. 

> 
> Also, do the plugin tests need to be updated? I know that we hardcode
> with-ast-visitor for the tests right now: should we update this flag?

Oops, done.

[vmpstr, 2016-02-17 00:53:40]

It seems I made that last part up about the check throwing more errors. I moved
the flag check lower. PTAL

[dcheng, 2016-02-17 06:09:14]

https://codereview.chromium.org/1703713002/diff/40001/tools/clang/plugins/Opt...
File tools/clang/plugins/Options.h (right):

https://codereview.chromium.org/1703713002/diff/40001/tools/clang/plugins/Opt...
tools/clang/plugins/Options.h:17: bool check_implicit_copy_ctors = false;
Let's add a comment about why this is needed and what the migration plan is for
removing this too.

[dcheng, 2016-02-17 06:09:49]

https://codereview.chromium.org/1703713002/diff/40001/tools/clang/plugins/Opt...
File tools/clang/plugins/Options.h (right):

https://codereview.chromium.org/1703713002/diff/40001/tools/clang/plugins/Opt...
tools/clang/plugins/Options.h:17: bool check_implicit_copy_ctors = false;
On 2016/02/17 at 06:09:14, dcheng wrote:
> Let's add a comment about why this is needed and what the migration plan is
for removing this too.

(For the migration plan, you can just link to the current bug)

[vmpstr, 2016-02-17 19:07:18]

PTAL

https://codereview.chromium.org/1703713002/diff/40001/tools/clang/plugins/Opt...
File tools/clang/plugins/Options.h (right):

https://codereview.chromium.org/1703713002/diff/40001/tools/clang/plugins/Opt...
tools/clang/plugins/Options.h:17: bool check_implicit_copy_ctors = false;
On 2016/02/17 06:09:49, dcheng wrote:
> On 2016/02/17 at 06:09:14, dcheng wrote:
> > Let's add a comment about why this is needed and what the migration plan is
> for removing this too.
> 
> (For the migration plan, you can just link to the current bug)

Done.

[dcheng, 2016-02-17 20:15:20]

LGTM.

https://codereview.chromium.org/1703713002/diff/60001/tools/clang/plugins/Opt...
File tools/clang/plugins/Options.h (right):

https://codereview.chromium.org/1703713002/diff/60001/tools/clang/plugins/Opt...
tools/clang/plugins/Options.h:18: // RecursiveASTVisitor approach. See
crbug.com/436357 for details.
Nit: https://crbug.com, it makes auto-linkification on codesearch work too.

[vmpstr, 2016-02-17 21:22:53]

https://codereview.chromium.org/1703713002/diff/60001/tools/clang/plugins/Opt...
File tools/clang/plugins/Options.h (right):

https://codereview.chromium.org/1703713002/diff/60001/tools/clang/plugins/Opt...
tools/clang/plugins/Options.h:18: // RecursiveASTVisitor approach. See
crbug.com/436357 for details.
On 2016/02/17 20:15:20, dcheng wrote:
> Nit: https://crbug.com, it makes auto-linkification on codesearch work too.

Done.

[vmpstr, 2016-02-17 21:33:12]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2016-02-17 21:33:12]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1703713002/#ps80001
(title: "+https")

[commit-bot: I haz the power, 2016-02-17 21:37:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1703713002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1703713002/80001

[commit-bot: I haz the power, 2016-02-17 22:00:50]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-17 22:02:03]

Description was changed from

==========
clang-plugin: Enable RecursiveASTVisitor approach by default.

This patch removes the ASTConsumer approach for checking TagDecls in
favor of always using RecursiveASTVisitor. This means that some of
the warnings have to be temporarily suppressed, since the ASTConsumer
approach was never checking them.

R=dcheng, thakis@chromium.org
BUG=436357
==========

to

==========
clang-plugin: Enable RecursiveASTVisitor approach by default.

This patch removes the ASTConsumer approach for checking TagDecls in
favor of always using RecursiveASTVisitor. This means that some of
the warnings have to be temporarily suppressed, since the ASTConsumer
approach was never checking them.

R=dcheng, thakis@chromium.org
BUG=436357

Committed: https://crrev.com/3d84fcc69061aac5c82ade4844749a4754ee5190
Cr-Commit-Position: refs/heads/master@{#375995}
==========

[commit-bot: I haz the power, 2016-02-17 22:02:04]

Patchset 5 (id:??) landed as
https://crrev.com/3d84fcc69061aac5c82ade4844749a4754ee5190
Cr-Commit-Position: refs/heads/master@{#375995}