Intent to Enable the Chromium Clang Style Checker

[Avi Drissman]

We have a custom Clang plugin in Chromium-land that enforces a variety of style rules on Chromium code. However, it has never enforced them on Blink.

Now that Blink is in the Chromium repository, I think it's a good idea to change that. I plan on changing the Blink code to get it into a state where the plugin is happy, and then turn on the enforcement.

Objections?

Avi

[Levi Weintraub]

Is that list you linked to comprehensive? If so, this seems like a good change.

Have you speculatively ran this over the Blink code? It seems like we should track the outstanding issues and burn those down so folks refactoring or working in code that already breaks this doesn't trigger the checker.

[Daniel Cheng]

Some of the checks are targeted at Chrome-specific things (base::WeakPtrFactory, base::RefCounted). Would we adapt those checks to also run against the Blink equivalents?

Daniel

[Avi Drissman]

No, I have not yet speculatively run it against the code. I'm patching the checker code now to see how bad it's going to be.

About adapting the checks, I would imagine that would be a good idea, but I'm considering that to be a second stage. The first stage is to get it building with the existing checks, and lock that in.

Avi

[Elliott Sprehn]

We intentionally have virtual inline methods. If you call super they will get inlined, also we intentionally make simple virtual overrides for boolean returns inline since it makes figuring out what a class does easier by scanning the header (the exact opposite of the example in the link).

Unless there's a measurable improvement on binary size or build time I don't think we should change that.

I can't say I've ever seen an issue where someone calls the destructor of a RefCounted manually either, having to add friends seems like unnecessary added ceremony, but I guess it's not too bad.

Can you post some example patches for what you have to change to enable this plugin? It's hard to say if this is good without understanding the churn it generates.

[Avi Drissman]

I built https://codereview.chromium.org/1372833002 on my Mac (with the enforce_in_thirdparty_webkit flag set to be true) and did a compile.

I'm attaching the entire build log from Blink compilation. Note that there are nine errors total.

Nine. Across all of Blink.

I'm shocked at that low number (and actually kinda proud that you peeps have kept it that clean), but given that, I find it hard to argue that we should have a different set of rules for Blink in this regard.

Avi

[Daniel Cheng]

It looks like that wasn't a complete build. Can you try using the "warn-only" option of the clang plugin, to get a list of all the warnings that we'd be emitting?

Daniel

[Avi Drissman]

Actually, it seems to be. If I switch the clang plugin to "warn-only" and also remove "-Werror" so that warnings aren't fatal, I can compile all the way up to content without hitting more than those nine issues.

Log attached.

Am I missing something?

Avi

[Daniel Cheng]

I'm not sure, but when I try the same thing locally (with https://codereview.chromium.org/user/a...@chromium.org patched in, and both flags enabled), I get 1.3 million warnings:

dcheng@nausicaa:~/src/chrome/src$ grep chromium-style build_logs  | wc -l

1367397

Daniel

[Avi Drissman]

OK, I'm seeing that too, now.

Lots of

../../third_party/WebKit/Source/core/fetch/ImageResourceClient.h:52:81: warning: [chromium-style] virtual methods with non-empty bodies shouldn't be declared inline.

../../third_party/WebKit/Source/core/animation/css/CSSTimingData.h:20:5: warning: [chromium-style] Complex destructor has an inline body.

../../third_party/WebKit/Source/core/animation/css/CSSAnimationData.h:14:1: warning: [chromium-style] Complex class/struct needs an explicit out-of-line destructor.

I don't have the reference at hand to the technical decisions that led us to add these warnings to the plugin, but if they applied to Chromium, they should be just as relevant to Blink.

Nico and friends, do you have the rationale that we were using here?

Avi

[Scott Graham]

On Mon, Sep 28, 2015 at 4:49 PM, 'Avi Drissman' via blink-dev <blin...@chromium.org> wrote:

OK, I'm seeing that too, now.

Lots of

../../third_party/WebKit/Source/core/fetch/ImageResourceClient.h:52:81: warning: [chromium-style] virtual methods with non-empty bodies shouldn't be declared inline.

../../third_party/WebKit/Source/core/animation/css/CSSTimingData.h:20:5: warning: [chromium-style] Complex destructor has an inline body.

../../third_party/WebKit/Source/core/animation/css/CSSAnimationData.h:14:1: warning: [chromium-style] Complex class/struct needs an explicit out-of-line destructor.

I don't have the reference at hand to the technical decisions that led us to add these warnings to the plugin, but if they applied to Chromium, they should be just as relevant to Blink.

Nico and friends, do you have the rationale that we were using here?

Some here: https://www.chromium.org/developers/coding-style/chromium-style-checker-errors#TOC-Constructor-Destructor-Errors

[Avi Drissman]

+Nico for reals

Scott: That's good; thank you.

Do we have records of how that affected build time or binary size? Elliott brought those up, and while I believe that these changes would help with both, I would like to bring some data to the party.

Avi

[Daniel Cheng]

A lot of the inline constructor/destructor warnings are just because we never had this warning in Blink. However, there are probably some instances that are deliberate for perf reasons: for example, uninlining WTF::String's ctor and dtor saved over 300KB in a release build the last time I checked, but I didn't have an opportunity to measure the perf impact.

Daniel

[Peter Kasting]

On Mon, Sep 28, 2015 at 4:55 PM, Scott Graham <sco...@chromium.org> wrote:

On Mon, Sep 28, 2015 at 4:49 PM, 'Avi Drissman' via blink-dev <blin...@chromium.org> wrote:

Nico and friends, do you have the rationale that we were using here?

Some here: https://www.chromium.org/developers/coding-style/chromium-style-checker-errors#TOC-Constructor-Destructor-Errors

Also https://www.chromium.org/developers/coding-style/cpp-dos-and-donts .

PK 

[Yuta Kitamura]

I'm sympathetic to what the Chromium style requires/recommends here, but we (Blink/WebKit) have been historically waaaay too lax about the use of inline functions. I believe there are tons of opportunities of perf/size improvements by de-inlining. Converting all the existing uses would be an extremely tough effort, but I think that that's a debt we'd like to pay eventually.

Maybe we'd like to try out de-inlining some major classes to see perf/size effects? However, there are many perf sensitive functions in such classes, so it'll be not very obvious to tell which to de-inline or not.