Adding more bugprone clang-tidy checks to Chromium [1/N]

[George Burgess]

Hey friends,

I've had cycles to evaluate a few more clang-tidy checks for inclusion in Chromium. Clang-tidy has a lot of checks spelled bugprone-*, so this is step one in a journey. ;)

This round, I looked at the following two checks:

- bugprone-unused-raii -- complains about patterns that look like unused RAII objects (e.g. `std::lock_guard(this->lock); use(this->protected_by_lock);`.) Outside of unittests, this flagged 15 places in Chromium; 12 are almost definitely bugs, and it's unclear if the remaining three are intended or not.

- bugprone-string-integer-assignment -- complains about the use of `std::string::operator=(int)` and similar. Of the ~17 unique pieces of code (there was much near-copy-paste in e.g., protobufs), it caught 6 things which are very likely to be bugs. The rest was in the shape `base::RandInt('a', 'z');`, `'a' + some_var`, ...

I think the first one's a pretty clear win. The false-positive rate of the second in existing code is a bit high for my liking, but I'd imagine that a broken string-integer-assignment would generally be quite visible, so it's likely harder for that to slip through unnoticed for a long period of time than something like an unused raii container.

Thoughts?

George

[Peter Kasting]

On Mon, Sep 28, 2020 at 5:01 AM George Burgess <gb...@chromium.org> wrote:

- bugprone-string-integer-assignment -- complains about the use of `std::string::operator=(int)` and similar. Of the ~17 unique pieces of code (there was much near-copy-paste in e.g., protobufs), it caught 6 things which are very likely to be bugs. The rest was in the shape `base::RandInt('a', 'z');`, `'a' + some_var`, ...

I claim that while

'a' + base::RandInt(0, 'z' - 'a')

...is not technically buggy, a better spelling would be to have Rand<T = int>() and then do:

base::Rand<char>('a', 'z')

(I just filed https://bugs.chromium.org/p/chromium/issues/detail?id=1132851 for this idea.)

So I think it's OK to flag this sort of thing.

With the protobuf stuff, it's basically a hand-rolled ASCII-only ToUpper/ToLower.  In a review this should generally get flagged for discussion about whether the author should have used the base functions for doing conversion (since locale-aware or not is an important factor).  So I'm OK with it firing on the protobuf code.

Given these, I'm happy with the things being flagged and think we should enable both of these.

Separate note: Are we keeping/should we keep the codebase-wide clang-tidy rules in sync with Tricium here?  Seems like we should?

PK

[Nico Weber]

First one lgtm; no opinion on the second.

I believe they use the same config file, so they should stay in sync automatically.

 

PK

--
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/CAAHOzFC7oNfuRjC9GHvidhaPJm829A4jSPs7dk6yH_3wb8a6qQ%40mail.gmail.com.

[Andrew Grieve]

Thanks for working on these! 

If it caught only 26 spots in the entire codebase, then I'd be surprised to find the false-positive rate to be annoying. +1 to enable.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAMGbLiGDvydcyKWd2wPiAXmaPne1XJU0rOL8-BwJNSn-Bzc5Jw%40mail.gmail.com.

[George Burgess]

Thanks everyone for the feedback! Since things seem positive, I'll plan to land these on Thursday if no one raises new concerns before then.

>> Separate note: Are we keeping/should we keep the codebase-wide clang-tidy rules in sync with Tricium here?  Seems like we should?

> I believe they use the same config file, so they should stay in sync automatically.

Yup yup. Tricium only provides cflags to clang-tidy invocations; config is picked up in the same way as if you ran `clang-tidy foo.c -- ${flags}` locally (aka clang-tidy will search $(dirname foo.c) for .clang-tidy, then ../, then ../../, etc.)

This has caused a tiny bit of confusion in the past with CLs with old base SHAs: if I have a CL based on origin/main~10000 and I upload it today, I'll get the clang-tidy check list as of ToT 10K commits ago.