[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:34
+                       hasSourceExpression(IsFloatExpr),
+                       unless(hasParent(castExpr()))).bind("cast"),
+      this);
----------------
courbet wrote:
> JonasToth wrote:
> > Given C++ is weird sometimes: Is there a way that a cast might imply an narrowing conversion?
> > 
> > ```
> > double value = 0.4;
> > int i = static_cast<float>(value);
> > ```
> > 
> > or 
> > 
> > ```
> > void some_function(int);
> > 
> > double d = 0.42;
> > some_function(static_cast<float>(d));
> > ```
> > would come to mind.
> > 
> > Nice to have, IMHO not necessary.
> Luckily that does not seem to be implicit:
> 
> ```
> void f(double value) {
>   int i = static_cast<float>(value);
> }
> ```
> 
> ```
> FunctionDecl 0x7fe5ffbd4150 <experimental/users/courbet/benchmark/toto.cc:1:1, line:3:1> line:1:6 f 'void (double)'
> |-ParmVarDecl 0x7fe5ffbd4088 <col:8, col:15> col:15 used value 'double'
> `-CompoundStmt 0x7fe5ffbd4380 <col:22, line:3:1>
>   `-DeclStmt 0x7fe5ffbd4368 <line:2:3, col:36>
>     `-VarDecl 0x7fe5ffbd4250 <col:3, col:35> col:7 i 'int' cinit
>       `-ImplicitCastExpr 0x7fe5ffbd4350 <col:11, col:35> 'int' <FloatingToIntegral>
>         `-CXXStaticCastExpr 0x7fe5ffbd4320 <col:11, col:35> 'float' static_cast<float> <NoOp>
>           `-ImplicitCastExpr 0x7fe5ffbd4308 <col:30> 'float' <FloatingCast>
>             `-ImplicitCastExpr 0x7fe5ffbd42f0 <col:30> 'double' <LValueToRValue>
>               `-DeclRefExpr 0x7fe5ffbd42b0 <col:30> 'double' lvalue ParmVar 0x7fe5ffbd4088 'value' 'double'
> ```
> 
> 
> 
The interesting one should get diagnosed. Could you add a test?


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:41
+      binaryOperator(
+          anyOf(hasOperatorName("+="), hasOperatorName("-=")),
+          hasLHS(hasType(isInteger())),
----------------
courbet wrote:
> JonasToth wrote:
> > I would really like to add all other calc-and-assign operations. At least "*=" and "/=".
> Makes sense, I've added `*=` and `/=`. All others (`%=`, `&=`, `<<=` and variations thereof)  trigger a compilation error if the RHS is a float (rightfully).
> 
> 
For floats that is true.

If we care about the `ìnt->char` casts later, they should be added.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455