[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:25
 
 // FIXME: Check double -> float truncation. Pay attention to casts:
 void NarrowingConversionsCheck::registerMatchers(MatchFinder *Finder) {
----------------
I think this comment is now outdated.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:42
+                       unless(hasParent(castExpr())),
+                       unless(isInTemplateInstantiation()))
+          .bind("cast"),
----------------
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.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:66
+
+struct IntegerRange {
+  bool Contains(const IntegerRange &From) const {
----------------
Please enclose that `struct` with an anonymous namespace


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:85
+  if (T->isFloatingPoint()) {
+    const unsigned PrecisionBits = llvm::APFloatBase::semanticsPrecision(
+        Context.getFloatTypeSemantics(T->desugar()));
----------------
please remove that `const` as its uncommon to qualify values in LLVM code.


================
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
----------------
Maybe repleace `One` with `1.0`? It sounds slightly weird with the following `One`s


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:118
+  if (ToType == FromType || ToType == nullptr || FromType == nullptr ||
+      ToType->isDependentType() || FromType->isDependentType())
+    return false;
----------------
what about value dependentness? That is resolveable, but i believe ignored here


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:125
+  if (FromType->isIntegerType() && ToType->isIntegerType())
+    return ToType->getScalarTypeKind() == clang::Type::STK_Bool
+               ? false
----------------
narrowing to `bool` is not nice either. I would prefer diagnosing these too.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:132
+
+  const auto FromIntegerRange = createFromType(Context, FromType);
+  const auto ToIntegerRange = createFromType(Context, ToType);
----------------
please no `const` and `auto` as well, as the type is not obvious.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:147
+                                                     const Expr *Rhs) {
+  const QualType LhsType = Lhs->getType();
+  const QualType RhsType = Rhs->getType();
----------------
please no `const`


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:149
+  const QualType RhsType = Rhs->getType();
+  if (Lhs->isValueDependent() || Rhs->isValueDependent() ||
+      LhsType->isDependentType() || RhsType->isDependentType())
----------------
you can shorten this condition with `isDependentType`


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:155
+    return false;
+  const auto LhsIntegerRange = createFromType(Context, getBuiltinType(Lhs));
+  if (!LhsIntegerRange.Contains(IntegerConstant))
----------------
please no `const`


================
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());
----------------
you could remove both temporaries and return directly and then ellide the braces.
If you don't like that please remove the `const`


================
Comment at: docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst:15
 
-We enforce only part of the guideline, more specifically, we flag:
- - All floating-point to integer conversions that are not marked by an explicit
-   cast (c-style or ``static_cast``). For example: ``int i = 0; i += 0.1;``,
-   ``void f(int); f(0.1);``,
- - All applications of binary operators where the left-hand-side is an integer
-   and the right-hand-size is a floating-point. For example:
-   ``int i; i+= 0.1;``.
+A narrowing conversion is currently defined as a conversion from:
+ - an integer to a narrower integer (e.g. ``char`` to ``unsigned char``),
----------------
Please reword that slightly. The narrowing conversions are defined by the standard, we just implement it "partially" or so.


================
Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:80
+  // We warn on the following even though it's not dangerous because there is no reason to use a double literal here.
+  // TODO(courbet): Provide an automatic fix.
+  f += 2.0;
----------------
llvm doesn't add a name to the TODOs.

The necessity to use the correct literal might still be there even if the number is in range: the precisions differ. I believe it could have differences on non-integer values what literal you use respectively.


================
Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:102
+    int i;       // 32 bits
+    float f = i; // doesn't fit in 24 bits
+    // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: narrowing conversion from 'int' to 'float' [cppcoreguidelines-narrowing-conversions]
----------------
could you please add tests with integer arithmetic?

E.g. this: 
```
short n1, n2;
float result = n1 + n2; // 'n1 + n2' is of type 'int' because of integer rules
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53488