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

Jonas Toth via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Nov 8 14:34:47 PST 2018

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?

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list