[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet marked an inline comment as done.
gchatelet added inline comments.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:42
+                       unless(hasParent(castExpr())),
+                       unless(isInTemplateInstantiation()))
+          .bind("cast"),
----------------
JonasToth wrote:
> Here and in the matcher below:
> 
> `isInTemplateInstantiation` is a wide range, if you match only on `isTypeDependent()` it would remove the false negatives from templates but still catch clear cases that are within a template function.
> 
> With the current implementation non-type-templates would be ignored as well.
> IMHO that could be done in a follow-up to keep the scope on one thing.
Sounds good, let's fix this in a follow-up patch.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:92
+
+    // 'One' is created with PrecisionBits plus two bytes:
+    // - One to express the missing negative value of 2's complement
----------------
JonasToth wrote:
> Maybe repleace `One` with `1.0`? It sounds slightly weird with the following `One`s
It's a leftover, I've reworded and changed the code as well.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:149
+  const QualType RhsType = Rhs->getType();
+  if (Lhs->isValueDependent() || Rhs->isValueDependent() ||
+      LhsType->isDependentType() || RhsType->isDependentType())
----------------
JonasToth wrote:
> you can shorten this condition with `isDependentType`
I tried using the following but it crashes when I run clang tidy other our codebase:
```
if (LhsType->isDependentType() || RhsType->isDependentType())
    return false;
```

Maybe I misunderstood what you suggested?


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:165
+  if (const auto *CO = llvm::dyn_cast<ConditionalOperator>(Rhs)) {
+    const bool NarrowLhs = diagIfNarrowConstant(
+        Context, CO->getLHS()->getExprLoc(), Lhs, CO->getLHS());
----------------
JonasToth wrote:
> you could remove both temporaries and return directly and then ellide the braces.
> If you don't like that please remove the `const`
I have to keep both variables or the first one might shortcircuit the second that goes undetected.
e.g. `unsigned char c = b ? 1 : -1;`


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53488