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

[Clement Courbet via Phabricator via cfe-commits]

courbet marked 2 inline comments as done.
courbet added a comment.

Thanks.



================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:32
+  Finder->addMatcher(
+      implicitCastExpr(hasImplicitDestinationType(isInteger()),
+                       hasSourceExpression(IsFloatExpr),
----------------
JonasToth wrote:
> Do you plan to check for `double` -> `float` conversion, too?
> 
> Having a limited scope for now is ok, but a FIXME would be nice.
I've added a FIXME.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:34
+                       hasSourceExpression(IsFloatExpr),
+                       unless(hasParent(castExpr()))).bind("cast"),
+      this);
----------------
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'
```





================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:41
+      binaryOperator(
+          anyOf(hasOperatorName("+="), hasOperatorName("-=")),
+          hasLHS(hasType(isInteger())),
----------------
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).




Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455