Clang-Tidy Lints, Episode II: Unification of the Ui/Views

[George Burgess]

(May the 4th be with you ;) )

Hello all!

To further our unification between the global .clang-tidy file and the one in ui/views, I'd like to discuss adding the following clang-tidy nits to our global listing. Please note that the links are links to evaluation docs with breakdowns of where these warnings appear/manual surveys of random samples of them/etc:

modernize-use-emplace -- recommends `emplace`-style functions over e.g., `vector::push_back` when the arg is an rvalue. The main benefit of this appears to be reduction in repetition, since we have a lot of `vec_of_foo.push_back(Foo(1, 2));`. This fires in 5,092 places over Chromium.

modernize-use-noexcept -- recommends `noexcept` over `throw()`. Given that we don't use exceptions much, this only fires in 57 places.

modernize-use-default-member-init -- this fires in 12,350 places over Chromium. It recommends

`struct Foo { int i = 1; Foo(); };`

over

`struct Foo { int i; Foo(): i(1) {} };`.

modernize-make-unique -- recommends the use of `foo = std::make_unique<Foo>();` over `foo = std::unique_ptr<Foo>(new Foo())`, `foo.reset(new Foo());`, and similar forms. This fires in 4,423 places over Chromium.

modernize-avoid-bind -- blanket recommendation to not use `std::bind`. This only fires in third_party, and has no special detection for base's Bind analogues. It complains about 72 uses of std::bind across Chromium.

----

Of these, I think modernize-use-default-member-init has the most room to go wrong, though I still think it's overall a value-add. I expand on this in the doc for it.

Meta: this should be the last round of tidy nits *purely* for ui/views unification. After this, I hope to get to `bugprone-`/`performance-` kinds of checks, which -- as one might imagine -- should be less style-intensive.

Thoughts appreciated,

George

[Peter Kasting]

I support these.

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/CA%2BrzOEn1eysJ4GLZ9JrJ0BVQkzqFiDGyHXYp4Bp2bg68gajYow%40mail.gmail.com.

[Marijn Kruisselbrink]

I'm curious how modernize-use-emplace relates to https://abseil.io/tips/112. Are there any cases where this check would go against what that tip recommends?

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

[K. Moon]

+1 to the concern about modernize-use-emplace.

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

[Jan Wilken Dörrie]

It looks like by default it would, the accompanying doc claims the check fires on this line, which is one of those cases where both push_back and emplace_back work, and thus ToTW 112 suggests to use push_back. However, it looks like the plugin has an option to turn this behavior off: https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-emplace.html#cmdoption-arg-ignoreimplicitconstructors

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-5-fEH%2BUhDzxfuzNQo2VQ%3DxaWYSECcdQoVyPdHso8s7rw%40mail.gmail.com.

[George Burgess]

+1 jdoerrie.

Glancing at the source, clang-tidy complains only when a constructor is being invoked, either explicitly or (if a bit is set) implicitly. As a special case, `std::make_{pair,tuple}` count as "constructors" here.

Distilling the ToTW down to its last sentence: "so in general, if both push_back() and emplace_back() would work with the same arguments, you should prefer push_back(), and likewise for insert() vs. emplace()." As jdoerrie noted, this check _can_ generally cause us to violate this ToTW when an implicit conversion is involved:

```

struct Foo { /* implicit */ Foo(int i); };

void bar(std::vector<Foo> &v) {

  v.push_back(0); // modernize-use-emplace says "use emplace here."

  Foo f;

  v.push_back(f); // not here

  v.push_back(std::move(f)); // nor here

}

```

We have a clang-tidy check in place that encourages devs to mark ctors callable with a single arg as `explicit`, which is consistent with Google style. Given code that follows that, it's not clear to me how this check can violate that ToTW. The closest I can get is something like:

```

struct Foo { explicit Foo(int i); };

void bar(std::vector<Foo> &v) {

  v.push_back(Foo{0}); // modernize-use-emplace says "use emplace here."

  v.emplace_back(0); // recommended code

}

```

I've no issue with turning this check off for implicit conversions so we can remain consistent with the linked ToTW's guidance.

[Peter Kasting]

On Mon, May 4, 2020 at 9:23 AM Marijn Kruisselbrink <m...@chromium.org> wrote:

I'm curious how modernize-use-emplace relates to https://abseil.io/tips/112. Are there any cases where this check would go against what that tip recommends?

TLDR: Yes, but that's fine with me.

Longer answer: I think it's possible to use emplace_back() too much, by using it anywhere it will compile (as people often do today, which is why I've sent mail in the past about not doing this, and fixed instances in the code).  I also think it's possible to use push_back() too much, by using it anywhere it will compile, which is what I think this particular TotW recommends.  I find its argument tenuous and its example uncompelling.

IMO you should use push_back() when you already have a type that you're copying/moving onto the element and emplace_back() when you are constructing something new.  That makes the function of the line maximally clear and maximally efficient (indeed, the distinction in the function of the TotW example is clear to me assuming that one knows that vector has the horrible API design of accepting a single argument that means "n null-constructed elements" -- that API design, not emplace_back(), is the reason for confusion and risk).  That is the usage of emplace_back() that this check suggests (though only from one direction; it doesn't also suggest emplace->push when no type conversion is occurring, which would be nice).

Therefore, I think the check is better than the TotW advice.

Now, what about turning off the implicit construction check?  I'm willing to live with this, but I think it's inferior to the default state.  There are many types in the STL that have implicit constructors where Google style would request explicit ones.  Flipping this bit means that the checker will not request the use of the clearer, more efficient emplace_back() in these cases.  I think that's a loss.

So, I vote for leaving the check on and leaving the "implicit constructors" disable bit set to zero.

PK

[George Burgess]

Alright, so summing up discussions so far:

modernize-use-emplace -- only concern is colliding with ToTW guidance, which can be 'fixed' by flipping a setting. Some disagreement about whether we should flip that setting.
modernize-use-noexcept -- no concerns raised yet.
modernize-use-default-member-init -- no concerns raised yet.
modernize-make-unique -- no concerns raised yet.
modernize-avoid-bind -- no concerns raised yet.

If I hear nothing about the last 4 by Monday, I'll consider that a green light to land them. As always, I'll follow up after a week or two with a "Not Useful" report for all of our new checks.

RE "what state should the bit be in for modernize-use-emplace," I agree with

> IMO you should use push_back() when you already have a type that you're copying/moving onto the element and emplace_back() when you are constructing something new

overall. I guess my only concern is that we don't have any form of automated push-back (...pun intended) against devs overusing emplace_back compared to this ideal. Tidy doesn't immediately seem to have anything about this, either, though it might be easy to add? I feel like there'd be value in making minimum noise (e.g., turning nits about implicit conversions off) until we can give great automated guidance on this, but maybe I'm overweighting it.

Do we give devs guidance on how closely to follow absl's ToTWs?

[Peter Kasting]

On Thu, May 14, 2020 at 12:01 PM George Burgess <gb...@chromium.org> wrote:

> IMO you should use push_back() when you already have a type that you're copying/moving onto the element and emplace_back() when you are constructing something new

overall. I guess my only concern is that we don't have any form of automated push-back (...pun intended) against devs overusing emplace_back compared to this ideal. Tidy doesn't immediately seem to have anything about this, either, though it might be easy to add? I feel like there'd be value in making minimum noise (e.g., turning nits about implicit conversions off) until we can give great automated guidance on this, but maybe I'm overweighting it.

Certainly starting with the flag set to "warn less" is a harmless conservative approach; it will warn on a strict subset of the cases I want it to warn on.  So I think that's a perfectly fine starting point.  I mainly want to make the warning more aggressive at some point if possible.

Do we give devs guidance on how closely to follow absl's ToTWs?

No.  I sometimes cite TotWs in reviews to back up a suggestion I'm making, but there's nothing in our docs that links to them as a group or suggests people should follow them.  They're on equal footing with e.g. Effective C++ or something: an external non-binding source whose opinions are worth considering.

PK

[George Burgess]

Following up here, since it's been ~a month since these landed.

The clang-tidy checks we landed were:

- modernize-bind
- modernize-make-unique
- modernize-use-default-member-init
- modernize-use-emplace
- modernize-use-noexcept

The only two of these that've fired since they were enabled are `modernize-use-default-member-init` and `modernize-use-emplace`. The rest had a very low number of occurrences in Chromium to start with, so it's unsurprising that they've been quiet. I don't think the "not useful" rates are large enough to reconsider the addition of these checks.

More details:

`modernize-use-default-member-init` has appeared on 76 pieces of code, and has been marked "not useful" 6 times. So, we have a 7.8% not useful rate (drops to 1.3% if my guesses below are right).

- 4 of the 6 were on lines "fuzzed" by Tricium. i.e., lines outside of the diff, but which still receive "while you're super close, can we also..." linting complaints. I think Chromium's setting for clang-tidy is 1 line of fuzz distance, so these often looked like

```

bool old_member_; // clang-tidy: hey please add `= false;` here, too.

bool newly_added_member = false;

```

Given that members are often grouped like this, I'm unsurprised that fuzzing catches things semi-often here :)

- 1 of the 6 was not useful presumably because a CL reviewer saw tidy's comment and suggested a better pattern overall (which included marking the member in question `const`, which means clang-tidy's suggestion wasn't possible to follow)

- In the final one, there was already a strong local style of a default ctor init'ing tons of constants out-of-line, e.g.:

```

struct Foo {

  Foo();  

  int m1, m2, m3, m4, m5;

};

Foo::Foo(): m1(0), m2(1), m3(2) /* ... */ {}

```

`modernize-use-emplace` has appeared on 15 pieces of code, and has been marked "not useful" 3 times. So, we have a low sample size 20% not-useful rate. Of these, 1 "not useful" wasn't addressed. So more like 6% maybe?

- On the first, the reviewer clicked "please fix", and it was fixed by the owner without complaint.

- The second was marked "Not useful" initially, then silently fixed in a later patch-set without prompting. Similar "not sure what happened there."

- The final one was the most interesting. The dev got two of these diagnostics in the same CL, and proceeded to fix one of them && mark the other "Not useful":

Not fixed:

foo.push_back(Bar{}); // suggested: foo.emplace_back();

Fixed:
foo.push_back({std::move(a), std::move(b)}); // suggested: foo.emplace_back(std::move(a), std::move(b));

I assume the complaint is `foo.emplace_back();` is too implicit about the construction of a `Bar{}`. I agree that visually, `foo.emplace_back();` is less pronounced than `foo.push_back(Bar{});`, but enough to warrant a special case? ¯\_(ツ)_/¯

Again, these rates don't seem highly problematic to me. Unless anyone feels differently, I'm happy to consider these landed now.

Thanks,

George