gchatelet updated this revision to Diff 171826. gchatelet marked 12 inline comments as done. gchatelet added a comment.
- Address comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 Files: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h docs/ReleaseNotes.rst docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
Index: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp =================================================================== --- test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp +++ test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp @@ -1,4 +1,5 @@ -// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t +// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t -- -- +// -target x86_64-unknown-linux float ceil(float); namespace std { @@ -9,12 +10,12 @@ namespace floats { struct ConvertsToFloat { - operator float() const { return 0.5; } + operator float() const { return 0.5f; } }; -float operator "" _Pa(unsigned long long); +float operator"" _float(unsigned long long); -void not_ok(double d) { +void narrow_double_to_int_not_ok(double d) { int i = 0; i = d; // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions] @@ -24,11 +25,11 @@ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions] i = ConvertsToFloat(); // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions] - i = 15_Pa; + i = 15_float; // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions] } -void not_ok_binary_ops(double d) { +void narrow_double_to_int_not_ok_binary_ops(double d) { int i = 0; i += 0.5; // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions] @@ -52,6 +53,137 @@ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions] } +double operator"" _double(unsigned long long); + +float narrow_double_to_float_return() { + return 0.5; + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions] +} + +void narrow_double_to_float_not_ok(double d) { + float f; + f = d; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions] + f = 0.5; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions] + f = 15_double; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions] +} + +void narrow_double_to_float_not_ok_binary_ops(double d) { + float f; + f += 0.5; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions] + f += d; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions] + // 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; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions] + + f *= 0.5; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions] + f /= 0.5; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions] + f += (double)0.5f; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions] +} + +void narrow_integer_to_floating() { + { + long long ll; // 64 bits + float f = ll; // doesn't fit in 24 bits + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: narrowing conversion from 'long long' to 'float' [cppcoreguidelines-narrowing-conversions] + double d = ll; // doesn't fit in 53 bits. + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: narrowing conversion from 'long long' to 'double' [cppcoreguidelines-narrowing-conversions] + } + { + 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] + double d = i; // fits in 53 bits. + } + { + short n1, n2; + float f = n1 + n2; // 'n1 + n2' is of type 'int' because of integer rules + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: narrowing conversion from 'int' to 'float' [cppcoreguidelines-narrowing-conversions] + } + { + short s; // 16 bits + float f = s; // fits in 24 bits + double d = s; // fits in 53 bits. + } +} + +void narrow_contant_to_integer() { + { + unsigned char c1 = 0; + unsigned char c2 = 255; + unsigned char c3 = -1; + // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: narrowing conversion from 'int' to 'unsigned char' [cppcoreguidelines-narrowing-conversions] + unsigned char c4 = 256; + // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: narrowing conversion from 'int' to 'unsigned char' [cppcoreguidelines-narrowing-conversions] + } + { + char c1 = -128; + char c2 = 127; + char c3 = -129; + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: narrowing conversion from 'int' to 'char' [cppcoreguidelines-narrowing-conversions] + char c4 = 128; + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: narrowing conversion from 'int' to 'char' [cppcoreguidelines-narrowing-conversions] + } + { + unsigned short c1 = 0; + unsigned short c2 = 65535; + unsigned short c3 = -1; + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: narrowing conversion from 'int' to 'unsigned short' [cppcoreguidelines-narrowing-conversions] + unsigned short c4 = 65536; + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: narrowing conversion from 'int' to 'unsigned short' [cppcoreguidelines-narrowing-conversions] + } + { + short c1 = -32768; + short c2 = 32767; + short c3 = -32769; + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: narrowing conversion from 'int' to 'short' [cppcoreguidelines-narrowing-conversions] + short c4 = 32768; + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: narrowing conversion from 'int' to 'short' [cppcoreguidelines-narrowing-conversions] + } +} + +void narrow_conditional_operator_contant(bool b) { + unsigned char c1 = b ? 1 : 0; + unsigned char c2 = b ? 1 : 256; + // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: narrowing conversion from 'int' to 'unsigned char' [cppcoreguidelines-narrowing-conversions] + unsigned char c3 = b ? -1 : 0; + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: narrowing conversion from 'int' to 'unsigned char' [cppcoreguidelines-narrowing-conversions] +} + +void casting_integer_to_bool_is_not_ok() { + int i; + while (i) { + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'int' to 'bool' [cppcoreguidelines-narrowing-conversions] + } + for (; i;) { + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'int' to 'bool' [cppcoreguidelines-narrowing-conversions] + } + if (i) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'int' to 'bool' [cppcoreguidelines-narrowing-conversions] + } +} + +void casting_float_to_bool_is_not_ok() { + float f; + while (f) { + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'float' to 'bool' [cppcoreguidelines-narrowing-conversions] + } + for (; f;) { + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'float' to 'bool' [cppcoreguidelines-narrowing-conversions] + } + if (f) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'bool' [cppcoreguidelines-narrowing-conversions] + } +} + void ok(double d) { int i = 0; i = 1; @@ -89,15 +221,19 @@ void template_context() { f(1, 2); + f(1, .5f); f(1, .5); + f(1, .5l); } #define DERP(i, j) (i += j) void macro_context() { int i = 0; DERP(i, 2); + DERP(i, .5f); DERP(i, .5); + DERP(i, .5l); } -} // namespace floats +} // namespace floats Index: docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst =================================================================== --- docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst +++ docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst @@ -10,13 +10,17 @@ This rule is part of the "Expressions and statements" profile of the C++ Core Guidelines, corresponding to rule ES.46. See -https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-narrowing. +https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es46-avoid-lossy-narrowing-truncating-arithmetic-conversions. -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;``. +We enforce only part of the guideline, more specifically, we flag narrowing conversions from: + - an integer to a narrower integer (e.g. ``char`` to ``unsigned char``), + - an integer to a narrower floating-point (e.g. ``uint64_t`` to ``float``), + - a floating-point to an integer (e.g. ``double`` to ``int``), + - a floating-point to a narrower floating-point (e.g. ``double`` to ``float``). +This check will flag: + - All narrowing 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 with a narrowing conversions. + For example: ``int i; i+= 0.1;``. Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -170,6 +170,11 @@ <clang-tidy/checks/readability-redundant-smartptr-get>` check does not warn about calls inside macros anymore by default. +- The :doc:`cppcoreguidelines-narrowing-conversions + <clang-tidy/checks/cppcoreguidelines-narrowing-conversions>` check now + detects narrowing conversions between floating-point types + (e.g. ``double`` to ``float``). + Improvements to include-fixer ----------------------------- Index: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h =================================================================== --- clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h +++ clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h @@ -28,6 +28,19 @@ : ClangTidyCheck(Name, Context) {} void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + void diagIfNarrower(const ASTContext &Context, SourceLocation BeginLoc, + SourceLocation SourceLoc, const Expr *Lhs, + const Expr *Rhs); + + bool diagIfNarrowConstant(const ASTContext &Context, SourceLocation SourceLoc, + const Expr *Lhs, const Expr *Rhs); + + bool diagIfNarrowConstantInConditionalOperator(const ASTContext &Context, + SourceLocation SourceLoc, + const Expr *Lhs, + const Expr *Rhs); }; } // namespace cppcoreguidelines Index: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp =================================================================== --- clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp +++ clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp @@ -9,60 +9,193 @@ #include "NarrowingConversionsCheck.h" #include "clang/AST/ASTContext.h" +#include "clang/AST/Type.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "llvm/ADT/APSInt.h" +#include "llvm/Support/Mutex.h" +#include <cstdint> using namespace clang::ast_matchers; namespace clang { namespace tidy { namespace cppcoreguidelines { -// FIXME: Check double -> float truncation. Pay attention to casts: void NarrowingConversionsCheck::registerMatchers(MatchFinder *Finder) { // ceil() and floor() are guaranteed to return integers, even though the type // is not integral. - const auto IsCeilFloorCall = callExpr(callee(functionDecl( - hasAnyName("::ceil", "::std::ceil", "::floor", "::std::floor")))); + const auto IsCeilFloorCallExpr = expr(callExpr(callee(functionDecl( + hasAnyName("::ceil", "::std::ceil", "::floor", "::std::floor"))))); - const auto IsFloatExpr = - expr(hasType(realFloatingPointType()), unless(IsCeilFloorCall)); - - // casts: + // Casts: // i = 0.5; // void f(int); f(0.5); - Finder->addMatcher(implicitCastExpr(hasImplicitDestinationType(isInteger()), - hasSourceExpression(IsFloatExpr), - unless(hasParent(castExpr())), - unless(isInTemplateInstantiation())) - .bind("cast"), - this); + Finder->addMatcher( + implicitCastExpr(hasImplicitDestinationType(builtinType()), + hasSourceExpression(hasType(builtinType())), + unless(hasSourceExpression(IsCeilFloorCallExpr)), + unless(hasParent(castExpr())), + unless(isInTemplateInstantiation())) + .bind("cast"), + this); // Binary operators: // i += 0.5; - Finder->addMatcher( - binaryOperator(isAssignmentOperator(), - // The `=` case generates an implicit cast which is covered - // by the previous matcher. - unless(hasOperatorName("=")), - hasLHS(hasType(isInteger())), hasRHS(IsFloatExpr), - unless(isInTemplateInstantiation())) - .bind("op"), - this); + Finder->addMatcher(binaryOperator(isAssignmentOperator(), + hasLHS(expr(hasType(builtinType()))), + hasRHS(expr(hasType(builtinType()))), + unless(hasRHS(IsCeilFloorCallExpr)), + unless(isInTemplateInstantiation()), + // The `=` case generates an implicit cast + // which is covered by the previous matcher. + unless(hasOperatorName("="))) + .bind("binary_op"), + this); } -void NarrowingConversionsCheck::check(const MatchFinder::MatchResult &Result) { - if (const auto *Op = Result.Nodes.getNodeAs<BinaryOperator>("op")) { - if (Op->getBeginLoc().isMacroID()) - return; - diag(Op->getOperatorLoc(), "narrowing conversion from %0 to %1") - << Op->getRHS()->getType() << Op->getLHS()->getType(); - return; +static const BuiltinType *getBuiltinType(const Expr *E) { + if (const Type *T = E->getType().getCanonicalType().getTypePtrOrNull()) + return T->getAs<BuiltinType>(); + return nullptr; +} + +namespace { + +struct IntegerRange { + bool Contains(const IntegerRange &From) const { + return (llvm::APSInt::compareValues(Lower, From.Lower) <= 0) && + (llvm::APSInt::compareValues(Upper, From.Upper) >= 0); + } + + bool Contains(const llvm::APSInt &Value) const { + return (llvm::APSInt::compareValues(Lower, Value) <= 0) && + (llvm::APSInt::compareValues(Upper, Value) >= 0); + } + + llvm::APSInt Lower; + llvm::APSInt Upper; +}; + +} // namespace + +static IntegerRange createFromType(const ASTContext &Context, + const BuiltinType *T) { + assert(T && "T must not be nullptr"); + if (T->isFloatingPoint()) { + unsigned PrecisionBits = llvm::APFloatBase::semanticsPrecision( + Context.getFloatTypeSemantics(T->desugar())); + // Contrary to two's complement integer, floating point values are + // symmetric and have the same number of positive and negative values. + // The range of valid integer for a floating point value is: + // [-2^PrecisionBits, 2^PrecisionBits] + + // Values are created with PrecisionBits plus two bytes: + // - One to express the missing negative value of 2's complement + // representation. + // - One for the sign. + llvm::APSInt UpperValue(PrecisionBits + 2, /*isUnsigned*/ false); + UpperValue.setBit(PrecisionBits); + llvm::APSInt LowerValue(PrecisionBits + 2, /*isUnsigned*/ false); + LowerValue.setBit(PrecisionBits); + LowerValue.setSignBit(); + return {LowerValue, UpperValue}; } - const auto *Cast = Result.Nodes.getNodeAs<ImplicitCastExpr>("cast"); - if (Cast->getBeginLoc().isMacroID()) + if (T->isSignedInteger()) + return {llvm::APSInt::getMinValue(Context.getTypeSize(T), false), + llvm::APSInt::getMaxValue(Context.getTypeSize(T), false)}; + if (T->isUnsignedInteger()) + return {llvm::APSInt::getMinValue(Context.getTypeSize(T), true), + llvm::APSInt::getMaxValue(Context.getTypeSize(T), true)}; + + llvm::errs() << "Unhandled type " << T->getName(Context.getPrintingPolicy()) + << "\n"; + llvm_unreachable("Unhandled type"); +} + +static bool IsNarrowerType(const ASTContext &Context, const BuiltinType *ToType, + const BuiltinType *FromType) { + if (ToType == FromType || ToType == nullptr || FromType == nullptr || + ToType->isDependentType() || FromType->isDependentType()) + return false; + + if (FromType->isRealFloatingType() && ToType->isIntegerType()) + return true; + + if (FromType->isIntegerType() && ToType->isIntegerType()) + return ToType->getKind() < FromType->getKind(); + + if (FromType->isRealFloatingType() && ToType->isRealFloatingType()) + return ToType->getKind() < FromType->getKind(); + + IntegerRange FromIntegerRange = createFromType(Context, FromType); + IntegerRange ToIntegerRange = createFromType(Context, ToType); + if ((FromType->isIntegerType() && ToType->isRealFloatingType())) + return !ToIntegerRange.Contains(FromIntegerRange); + + llvm::errs() << "Unhandled from type " + << FromType->getName(Context.getPrintingPolicy()) << "or to type" + << ToType->getName(Context.getPrintingPolicy()) << "\n"; + llvm_unreachable("Unhandled case"); +} + +bool NarrowingConversionsCheck::diagIfNarrowConstant(const ASTContext &Context, + SourceLocation SourceLoc, + const Expr *Lhs, + const Expr *Rhs) { + QualType LhsType = Lhs->getType(); + QualType RhsType = Rhs->getType(); + if (Lhs->isValueDependent() || Rhs->isValueDependent() || + LhsType->isDependentType() || RhsType->isDependentType()) + return false; + llvm::APSInt IntegerConstant; + if (!Rhs->isIntegerConstantExpr(IntegerConstant, Context)) + return false; + IntegerRange LhsIntegerRange = createFromType(Context, getBuiltinType(Lhs)); + if (!LhsIntegerRange.Contains(IntegerConstant)) + diag(SourceLoc, "narrowing conversion from %0 to %1") << RhsType << LhsType; + return true; +} + +bool NarrowingConversionsCheck::diagIfNarrowConstantInConditionalOperator( + const ASTContext &Context, SourceLocation SourceLoc, const Expr *Lhs, + const Expr *Rhs) { + if (const auto *CO = llvm::dyn_cast<ConditionalOperator>(Rhs)) { + // We create variables so we make sure both sides of the + // ConditionalOperator are evaluated, the || operator would shortciruit + // the second call otherwise. + bool NarrowLhs = diagIfNarrowConstant(Context, CO->getLHS()->getExprLoc(), + Lhs, CO->getLHS()); + bool NarrowRhs = diagIfNarrowConstant(Context, CO->getRHS()->getExprLoc(), + Lhs, CO->getRHS()); + return NarrowLhs || NarrowRhs; + } + return false; +} + +void NarrowingConversionsCheck::diagIfNarrower(const ASTContext &Context, + SourceLocation BeginLoc, + SourceLocation SourceLoc, + const Expr *Lhs, + const Expr *Rhs) { + if (BeginLoc.isMacroID()) + return; + if (diagIfNarrowConstantInConditionalOperator(Context, SourceLoc, Lhs, Rhs)) return; - diag(Cast->getExprLoc(), "narrowing conversion from %0 to %1") - << Cast->getSubExpr()->getType() << Cast->getType(); + if (diagIfNarrowConstant(Context, SourceLoc, Lhs, Rhs)) + return; + if (IsNarrowerType(Context, getBuiltinType(Lhs), getBuiltinType(Rhs))) + diag(SourceLoc, "narrowing conversion from %0 to %1") + << Rhs->getType() << Lhs->getType(); +} + +void NarrowingConversionsCheck::check(const MatchFinder::MatchResult &Result) { + if (const auto *Op = Result.Nodes.getNodeAs<BinaryOperator>("binary_op")) + return diagIfNarrower(*Result.Context, Op->getBeginLoc(), + Op->getOperatorLoc(), Op->getLHS(), Op->getRHS()); + if (const auto *Cast = Result.Nodes.getNodeAs<ImplicitCastExpr>("cast")) + return diagIfNarrower(*Result.Context, Cast->getBeginLoc(), + Cast->getExprLoc(), Cast, Cast->getSubExpr()); + llvm_unreachable("must be binary operator or cast expression"); } } // namespace cppcoreguidelines
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits