Page MenuHomePhabricator

[clang-tidy] Catch more unwanted implicit conversions in performance-implicit-conversion-in-loop
Needs ReviewPublic

Authored by courbet on Nov 26 2020, 6:48 AM.



It turns out that there is no reason to restrict to loop variables. This
allows catching stuff like:

const std::pair<uint32, SomeLargeObject>& kv = *map.begin();

(did you notice the missing const uint32_t) ?

On our codebase, this roughly doubles the number of warnings compared to
the loop-only case.

The warnings mainly fall in two categories:

  1. Lifetime extended temporary:
const std::function<...>& c = [](...) {};

Which is better written as one of:

const std::function<...> c = [](...) {};  // explicit
const auto c = [](...) {};  // no std::function
  1. Copy + lifetime extension.
const std::pair<uint32, SomeLargeObject>& kv = *map.begin();

Better written as:

// no copy:
const std::pair<const uint32, SomeLargeObject>& kv = *map.begin();
const auto& kv = *map.begin();
// explicit
const std::pair<uint32, SomeLargeObject> kv = *map.begin();
// explicit, allows automatic move.
std::pair<uint32, SomeLargeObject> kv = *map.begin();

And rename the check.

Diff Detail

Event Timeline

courbet created this revision.Nov 26 2020, 6:48 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 26 2020, 6:48 AM
courbet requested review of this revision.Nov 26 2020, 6:48 AM

Please mention renaming in Release Notes.


Please use space after clang-tidy.


Please fix length and use space after clang-tidy.

Eugene.Zelenko edited projects, added Restricted Project; removed Restricted Project.
Eugene.Zelenko removed a subscriber: llvm-commits.
Herald added a project: Restricted Project. · View Herald TranscriptNov 26 2020, 7:00 AM
courbet updated this revision to Diff 307863.Nov 26 2020, 7:17 AM

Add to release notes.

courbet marked 2 inline comments as done.Nov 26 2020, 7:18 AM

Thanks, comments addressed.

aaron.ballman added inline comments.Dec 10 2020, 7:19 AM

This check has been around since at least 2016 -- are we sure we want to rename it? @alexfh -- is this a case where we should leave the old check name as an alias?

Also, as we generalize the check, it starts to be a question of whether this should be in the frontend under one of the -Wconversion flags (or a new one).