Adding more default clang-tidy lints to Chromium

[George Burgess]

Hey,

So now that we're running clang-tidy on CLs, it's my goal to get clang-tidy to complain more: crbug.com/1016926

Pursuant to this, I'd like to invoke the time-honored tradition of adding global clang-tidy checks to Chromium, which I established about five minutes ago. ;) 

For context, Tricium -- the machinery we're using to surface these during code review -- has a "Not Useful" button that we can use to determine if devs dislike any lints we allow here. Additionally, these nits are only shown on changed LOC in a patch, so the hope is that most complaints in-tree today will slowly go away over time.

There are probably over 20 lints I'd like to add in total, but for now, I'd like to start with 5. These are all already enabled for ui/views, and I don't think they'll be contentious, so batching seems fine to me. We'll see if I'm right. :)

Concretely, in this round, I'd like to propose that we enable:

- modernize-loop-convert (which fires 5.6K times throughout Chromium)

- modernize-make-shared (which fires once in all of Chromium)

- readability-redundant-member-init (which fires 1.7K times)

- google-explicit-constructor (1K occurrences)

- modernize-use-transparent-functors (53 occurrences)

A quick description of what each does + each lint's benefits:

- loop-convert encourages devs to use C++11's range for loops over `for (T index = 0; index < ...)`, which is sometimes a small performance/code size win (since `end()` is "cached" at the beginning of the loop), and is ~always a clarity win.

- modernize-make-shared encourages the use of std::make_shared over shared_ptr's ctor. It's a performance win in non-pathological cases, and is considered good practice ~everywhere AIUI.

- readability-redundant-member-init is a clarity win, since it makes member initializers have meaning.

- google-explicit-constructor encourages consistency with Google's style guide (thus Chromium's), and can make subtle bugs more obvious.

- modernize-use-transparent-functors encourages the use of std::less<> et al, instead of std::less<T>. This makes code more concise and less prone to implicit conversions (introduced at the time of writing, or introduced due to an incomplete update down the line.

Thoughts?

George

[David Munro]

Does modernize-make-shared work for scoped_refptr which IIUC chromium uses instead of shared_ptr?

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CA%2BrzOEkmbeUgwixRAPQBeC2VBH2%3DbQ4F3rBinuSAzD2wfMRzZw%40mail.gmail.com.

[Peter Kasting]

On Tue, Dec 17, 2019 at 6:47 PM David Munro <david...@google.com> wrote:

Does modernize-make-shared work for scoped_refptr which IIUC chromium uses instead of shared_ptr?

No, or it would fire more than once, since we have far more than one place that needs to be converted from scoped_refptr() to MakeRefCounted() :)

PK 

[Peter Kasting]

On Tue, Dec 17, 2019 at 6:24 PM George Burgess <gb...@chromium.org> wrote:

Thoughts?

I'm not going to complain about anything views has already turned on.  I do wonder how you chose this particular mix of five; it's not consistently the most or least-impacting set.  Does it make sense to first turn on all the smallest things before we get to things like modernize-loop-convert?

PK

[Anton Bikineev]

I like the idea of having more checks in clang-tidy bot!

I've recently added a performance-trivially-destructible check, which removes out-of-line dtor declarations in case that would make a class trivially-destructible. This is essential for GC in Blink, which keeps and executes destructors based on std::is_trivially_destructible trait. I was thinking of duplicating this check in the clang plugin, but that would require changing ~2000 places in chrome. Doing that in a separate bot would be much easier. Would it be possible to add this one too?

[George Burgess]

> I do wonder how you chose this particular mix of five; it's not consistently the most or least-impacting set

My feelings, basically. :)

A side-focus of this thread is establishing/solidifying this whole 'adding new lints' process, so I picked things that I felt would be the least contentious lints to maximize room on the thread for folx to express more meta thoughts/suggestions. I also wanted at least one that's likely to be hit by a fair number of devs soon, so we could start exploring how to look at the 'Not Useful' metrics that're a thing here.

Going smallest->largest would give us:

- modernize-make-shared (1)

- modernize-avoid-bind (3)

- modernize-use-noexcept (22)

- modernize-use-transparent-functors (53)

- google-readability-namespace-comments (475)

If you think it's better to target those first, I think we'll get to the union of my list + yours within the next month or so anyway, so it's up to you (assuming no one objects).

[George Burgess]

Glad to hear it!

I'm personally cool with that check, but can we please pull that discussion into a separate email thread? :)

[Nico Weber]

On Wed, Dec 18, 2019 at 1:24 PM George Burgess <gb...@chromium.org> wrote:

> I do wonder how you chose this particular mix of five; it's not consistently the most or least-impacting set

My feelings, basically. :)

A side-focus of this thread is establishing/solidifying this whole 'adding new lints' process, so I picked things that I felt would be the least contentious lints to maximize room on the thread for folx to express more meta thoughts/suggestions. I also wanted at least one that's likely to be hit by a fair number of devs soon, so we could start exploring how to look at the 'Not Useful' metrics that're a thing here.

Going smallest->largest would give us:

- modernize-make-shared (1)

- modernize-avoid-bind (3)

- modernize-use-noexcept (22)

- modernize-use-transparent-functors (53)

- google-readability-namespace-comments (475)

Is this a copy-pasto? These are different from the 5 in the initial mail.

I'm pretty sure we don't want google-readability-namespace-comments since clang-format adds those comments. I feel clang-tidy stuff should be for things that a good reviewer would call out ("did you mean to do this thing?"), not things that can be automatically fixed by a tool.

I don't remember the conclusion of the last noexcept thread, but iirc it was "don't go overboard with it".

For the original list: The first 3 seem ok, but similar to pkasting they look a bit arbitrary to me.

I'm not sure how useful the 4th is with C++11 uniform initialization making multi-arg explicit ctors a thing.

The fifth looks ok, but pretty rare.

On the meta level of this thread:

Imho it'd be cool to focus on checks that catch logic errors and save devs work first, instead of focusing on checks that add style nits :) To me, that seems like something people would appreciate more in their robo-review comments. Several of the bugprone- checks look very useful, eg https://clang.llvm.org/extra/clang-tidy/checks/bugprone-use-after-move.html

Anecdotally from seeing these checks getting implemented, I'd expect that different checks have different levels of production readiness. Should we just enable checks hoping that they'll work fine and relying on "Not useful" clicks, or should we do some pre-enablement checking?

[George Burgess]

> Is this a copy-pasto? These are different from the 5 in the initial mail.

pkasting's comment was that we might want to go from least -> most number of occurrences seen on the bug; that's what the list would look like if we took that approach instead.

> Imho it'd be cool to focus on checks that catch logic errors and save devs work

Oh, totally. Enabling nearly everything spelled `bugprone-*` + many things from `performance-*` is definitely something I want to do + what I think has the highest value here -- I did an analysis some months ago on distributions and such, and produced a stream-of-consciousness-ish-doc here. 

Again, since the listing in crbug.com/1016926#c4 is basically what ui/views has enabled but that we don't have globally, I'd assumed:

- they'd generally be things we want globally (which could definitely be wrong. Y'all have leagues more context on Chromium's C++ idioms/style/etc than I do. :) )

- it'd be ideal to remove ui/views' .clang-tidy, since AIUI the checks there won't be additive to whatever we do in src/.clang-tidy.

> Should we just enable checks hoping that they'll work fine and relying on "Not useful" clicks, or should we do some pre-enablement checking?

Concretely, this would mean sampling a random, say, 25-40 occurrences of each check and reporting on which ones were false-positives? I'd be cool with that.

[Daniel Cheng]

Is it possible to just run clang-tidy across all of Chrome (even if it's only one config) and get a rough idea of the value of different checks? I think we had something like this for clang's static analysis when we were experimenting with it.

Daniel

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CA%2BrzOEkzQwemA7mQL02M2F-B3xpYuqmJhNdsSa3kOG03UJJrBg%40mail.gmail.com.

[George Burgess]

Which ones? The ones in the doc all have a total number of occurrences across the entire tree (as of August or so of this year) next to them, and the ones presented here have numbers as of earlier this week next to them. (e.g., modernize-loop-convert is hit ~5,600 times, ...)

[George Burgess]

Happy (belated) New Year! 🎆

So to summarize so far, the below checks are up for review. Per the request for a random survey of each check's diagnostics, I've added docs that survey some of the checks:

- modernize-loop-convert (which fires 5.6K times throughout Chromium; 0 oddities found)
- modernize-make-shared (which fires once in all of Chromium; 0 oddities found, though davidmunro noted that we might get more value out of having a scoped_refptr check similar to this one)
- readability-redundant-member-init (which fires 1.4K times; 0 oddities found)
- google-explicit-constructor (1K occurrences; 3 awkward suggestions found)
- modernize-use-transparent-functors (53 occurrences; 1 functional bug + 1 awkward diagnostic found)

Of those, the only uncertainty voiced so far is about the utility of google-explicit-constructor. AIUI, that check helps keep us consistent with Google's C++ style. Pulling out the relevant parts:

"""

Type conversion operators, and constructors that are callable with a single argument, must be marked `explicit` in the class definition. [...] Implicit conversions can sometimes be necessary and appropriate for types that are designed to transparently wrap other types. In that case, contact your project leads to request a waiver of this rule.

Constructors that cannot be called with a single argument may omit `explicit`. [...]

"""

I... don't think Chromium has any process that involves style guide waivers, and did notice 'wrapper' types being flagged by this rule. Depending on what folx here think about automated style nits like this, that may be a valid reason to drop this. I'm in the camp of "implicit conversions often do more harm than good," and users can always NOLINT their code, so if this does the right thing the strong majority of the time, I'm personally in favor of it.

[Peter Kasting]

On Thu, Jan 9, 2020 at 1:59 PM George Burgess <gb...@chromium.org> wrote:

I... don't think Chromium has any process that involves style guide waivers,

We do, you can just ask cxx@.  (Or, in the case of a few places in ui/, you can be one of the cxx@ members and make an executive decision to intentionally violate the rule.)

 

and did notice 'wrapper' types being flagged by this rule. Depending on what folx here think about automated style nits like this, that may be a valid reason to drop this. I'm in the camp of "implicit conversions often do more harm than good," and users can always NOLINT their code, so if this does the right thing the strong majority of the time, I'm personally in favor of it.

Something that suggests rather than requiring, and is almost always correct, is still a good check.  I'm in favor of this one.

PK 

[George Burgess]

SGTM.

I'm not hearing much push-back, so I plan to land the following checks around EOD (PST) on Monday. If there are any remaining objections/concerns, please speak up.

- modernize-loop-convert
- modernize-make-shared
- readability-redundant-member-init
- google-explicit-constructor
- modernize-use-transparent-functors

After those have had some time to settle, I'll be back for round two :)

Lastly, again, since this is a newish process, any feedback on it is also appreciated!

Thank you all,

George

[George Burgess]

Happy Wednesday, friends!

Following up, here are the metrics I've collected for these lints since we enabled them last month:

- modernize-loop-convert fired 301 times

- modernize-make-shared fired 0 times
- readability-redundant-member-init fired 79 times
- google-explicit-constructor fired 128 times
- modernize-use-transparent-functors fired 9 times

(where "fired" means "actually showed up as a robocomment on a CL.")

For each of those, users clicked "not useful" the following number of times:

- modernize-loop-convert -- 11 (3.6% of lints)
- modernize-make-shared -- 0 (0% of lints)
- readability-redundant-member-init -- 1 (1.2% of lints)
- google-explicit-constructor -- 4 (3.1% of lints)
- modernize-use-transparent-functors -- 0 (0% of lints)

Since there aren't many "not useful"s here, I'll investigate the individual comments offline to see if any of these were the result of clang-tidy bugs.

Even if not, I think these numbers are good enough that these lints should all stick.

George

[Peter Kasting]

On Wed, Feb 12, 2020 at 12:52 PM George Burgess <gb...@chromium.org> wrote:

Following up, here are the metrics I've collected for these lints since we enabled them last month:

- modernize-loop-convert fired 301 times

- modernize-make-shared fired 0 times
- readability-redundant-member-init fired 79 times
- google-explicit-constructor fired 128 times
- modernize-use-transparent-functors fired 9 times

(where "fired" means "actually showed up as a robocomment on a CL.")

For each of those, users clicked "not useful" the following number of times:

- modernize-loop-convert -- 11 (3.6% of lints)
- modernize-make-shared -- 0 (0% of lints)
- readability-redundant-member-init -- 1 (1.2% of lints)
- google-explicit-constructor -- 4 (3.1% of lints)
- modernize-use-transparent-functors -- 0 (0% of lints)

Since there aren't many "not useful"s here, I'll investigate the individual comments offline to see if any of these were the result of clang-tidy bugs.

Even if not, I think these numbers are good enough that these lints should all stick.

These are solid numbers.  The only one that worries me a little is modernize-loop-convert, because I know it sometimes suggested transforms that weren't instantly obvious.  That's the one I'd look closely at,

Thanks for spearheading this work.

PK 

[Nico Weber]

On Wed, Feb 12, 2020 at 3:52 PM George Burgess <gb...@chromium.org> wrote:

Happy Wednesday, friends!

Following up, here are the metrics I've collected for these lints since we enabled them last month:

- modernize-loop-convert fired 301 times

- modernize-make-shared fired 0 times
- readability-redundant-member-init fired 79 times
- google-explicit-constructor fired 128 times
- modernize-use-transparent-functors fired 9 times

(where "fired" means "actually showed up as a robocomment on a CL.")

For each of those, users clicked "not useful" the following number of times:

- modernize-loop-convert -- 11 (3.6% of lints)
- modernize-make-shared -- 0 (0% of lints)
- readability-redundant-member-init -- 1 (1.2% of lints)
- google-explicit-constructor -- 4 (3.1% of lints)
- modernize-use-transparent-functors -- 0 (0% of lints)

Since there aren't many "not useful"s here, I'll investigate the individual comments offline to see if any of these were the result of clang-tidy bugs.

Could you maybe also look at a random sample of where one diag fired at all, and check where you would've clicked "not useful", so that we get a baseline on click-through rate for "not useful"? (I could imagine that people sometimes don't find a diag useful, but aren't bothered enough to click the "not useful" button.)

[George Burgess]

I had a feeling it'd be a mistake to send the email first and investigate later :)

So I looked into every 'not useful' above. Only 3 of them appear to be complaints about the actual clang-tidy check; the rest are more likely "no commentary on the clang-tidy check itself; just that it shouldn't be firing here". For example, 5 of the instances were not useful because of crbug.com/1044592 .

If my guesses about dev intent are correct, the actual "I don't think this category of checks is useful" numbers are:

- google-explicit-constructor -- 2 (both on the same CL)

- readability-redundant-member-init -- 1

So, yay for having even better numbers than we thought? 

> The only one that worries me a little is modernize-loop-convert, because I know it sometimes suggested transforms that weren't instantly obvious

Yeah, that check can be pretty aggressive; all of the instances it fired in boiled down to the following, though:

```

for (size_t i = 0; i < base::size(foo); i++) {

  // standard loop body using foo[i]

}

```

I'd imagine all of the complaints about that check were due to how Tricium handles metrics (either the macro bug above, or the fuzzy matching experiment mentioned below)

> Could you maybe also look at a random sample of where one diag fired at all, and check where you would've clicked "not useful", so that we get a baseline on click-through rate for "not useful"?

I checked ~40 links (my sample size was 50, but some 404'd), and my answer here is "It Depends(TM)."

Zero of the actual nitpicks I surveyed looked "not useful" to me in this sample. There were a few potentially borderline ones. Notably:

```

Foo::~Foo() {} // tidy: use `= default;`

```

Since I'd normally expect that to only appear in a header, though IIRC, we have guidance that sounds like "if some struct has N members, define ctors/dtors out-of-line," so the location of these nits seems reasonable to me.

That said, the number of things that _should've_ been marked "not useful" depends on your opinion on an in-progress experiment IMO. The Tricium folks are currently evaluating "fuzzy matching," so lines a few lines above/below those actually being touched might receive comments (crbug.com/1044846). I'm personally not a fan of this, but some people believe it may be useful, so we'll see how that shakes out.

Around half of the comments I surveyed were on lines not directly being touched by a diff. If most devs think that's good, our click-through rate seems pretty good. If most devs don't, then our click-through rate seems... less good. ¯\_(ツ)_/¯