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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
