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