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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:149
+  const QualType RhsType = Rhs->getType();
+  if (Lhs->isValueDependent() || Rhs->isValueDependent() ||
+      LhsType->isDependentType() || RhsType->isDependentType())
----------------
gchatelet wrote:
> 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?
You understood correct, but I was wrong. Could you please try `LhsType->isInstantiationDependentType()`, as this should involve all kind of template surprises.


================
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());
----------------
gchatelet wrote:
> 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;`
Indeed, no problem.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53488