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

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet added inline comments.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:178
+      return;
+    // Conversions to unsigned integer are well defined and follow modulo 2
+    // arithmetic.
----------------
JonasToth wrote:
> 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?
An Option to warn on these sound like a good idea.
I added TODOs.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53488