Switch the Clang plugin to use RecursiveASTVisitor.

Implement the Clang plugin as a RecursiveASTVisitor.

The current ASTConsumer class checker can be non-deterministic
in its class verification. The exact reason isn't clear, but it's
kind of nice to have a deterministic checker.

Using RecursiveASTVisitor also makes it easier to write more
interesting checks if needed in the future, since it also visits
statements, etc. Since the RecursiveASTVisitor implementation
catches strictly more things than the ASTConsumer version, it's
gated behind a flag. This flag/the legacy ASTConsumer version
will be removed once Chromium code can compile cleanly with the
AST visitor.

BUG=436357
Committed: https://crrev.com/80f54dc1ab629289946237227e5706e19b7e7594
Cr-Commit-Position: refs/heads/master@{#310331}

[dcheng, 2014-12-23 00:10:27]

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

[dcheng, 2014-12-23 00:21:40]

I don't know how interested we are in this. Maybe the current behavior is fine;
of the new warnings generated by the RecursiveASTVisitor version, the
CallbackBase copy constructor was the only real big win (the other instances I
tested were still size wins, but much smaller wins). It is nice to have more
determinism though...

[Nico, 2014-12-23 00:25:47]

Won't the RAV visit all substatements of a decl? Does this have any build time
impact?

[dcheng, 2014-12-23 00:27:33]

We could probably get it to short-circuit out by returning false in VisitStmt().
Let me try to quantify the build time impact though.

[dcheng, 2014-12-23 21:08:27]

My highly scientific testing method required commenting out the warnings we'd
normally emit for inline constructors and copy constructors. Hopefully this
doesn't invalidate the test results too much:

Testing methodology:
ninja -C out/Debug -t clean
/usr/bin/time --verbose ninja -C out/Debug chrome

2 runs without RAV: wall clock time: ~22:27, user time: ~39500s
2 runs with RAV: wall clock time: ~22:15, user time: ~39775s

There was little variation across runs, so I didn't bother doing more runs than
this. So the impact seems to be minimal.

[dcheng, 2015-01-06 19:18:56]

Ping =)

[brucedawson, 2015-01-06 19:30:25]

In the description you say "can compile cleanly the AST visitor." -- should that
be "can compile cleanly *with* the AST visitor."?

[brucedawson, 2015-01-06 19:33:57]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2015-01-06 19:33:58]

I'm not familiar with clang plugins but the code seems clear and reasonable
enough. Deferring to others for approval.

[dcheng, 2015-01-06 19:34:00]

On 2015/01/06 at 19:30:25, brucedawson wrote:
> In the description you say "can compile cleanly the AST visitor." -- should
that be "can compile cleanly *with* the AST visitor."?

Oops, I often words. Fixed.

[Nico, 2015-01-06 19:48:41]

It's behind a flag, you checked that it doesn't regress build time, and the
resident expert (also you) says that this is the way to go – lgtm :-)

[dcheng, 2015-01-07 18:12:05]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2015-01-07 18:13:00]

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

[commit-bot: I haz the power, 2015-01-07 19:14:17]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-07 19:15:59]

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