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