[PATCH] D53488: [clang-tidy] Improving narrowing conversions

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:178
+      return;
+    // Conversions to unsigned integer are well defined and follow modulo 2
+    // arithmetic.
----------------
gchatelet wrote:
> JonasToth wrote:
> > I am surprised by `following modulo 2 arithmetic` and think it's a bit misleading. Writing just `module arithmetic` is probably better, as `module 2` somewhat implies there a only 2 valid values (0, 1).
> > 
> > Is this the `int` -> `unsigned int` case path? That seems worth diagnosing too.
> Yes, thx for noticing. I updated the comment, I think it's better now.
> 
> Indeed this is the `int` -> `unsigned int` case path. Warning here would lead to a lot of noise for everybody doing bitwise operations since `-1` is a compact way to represent the maximum value. Since the semantic is valid and well defined by the standard I'm unsure it's worth the pain. I'm open to suggestions though.
> 
> Maybe a good way to figure out is what would be the correct fix for say `unsigned long AllBits = -1;`
Comment is fine :)

I though that we have check that diagnoses `unsigned i = -1;` but I don't find it right now (maybe its still in review or so, i belive it was related to introducing `std::numeric_limits<>`).
As its well defined and not narrowing and not mentionend by the CPPCG in that section its ok, maybe worth an option in the future?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53488