gchatelet updated this revision to Diff 170878. gchatelet marked an inline comment as done. gchatelet added a comment.
- Join the two parts of the ReleaseNotes update 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 @@ -9,12 +9,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 +24,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 +52,43 @@ // 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 = 0; + 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 = 0; + 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 ok(double d) { int i = 0; i = 1; @@ -89,15 +126,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,15 @@ 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;``. +A narrowing conversion is currently defined as a conversion from: + - 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 @@ -163,6 +163,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,13 @@ : ClangTidyCheck(Name, Context) {} void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + void checkBinaryOperator(const ASTContext *const Context, + const BinaryOperator &Op); + + void checkCastOperator(const ASTContext *const Context, + const ImplicitCastExpr &Cast); }; } // namespace cppcoreguidelines Index: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp =================================================================== --- clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp +++ clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp @@ -27,42 +27,68 @@ 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( + anyOf(hasImplicitDestinationType(isInteger()), + hasImplicitDestinationType(realFloatingPointType())), + hasSourceExpression(IsFloatExpr), 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"), + binaryOperator( + isAssignmentOperator(), + // The `=` case generates an implicit cast which is covered + // by the previous matcher. + unless(hasOperatorName("=")), + hasLHS(anyOf(hasType(isInteger()), hasType(realFloatingPointType()))), + hasRHS(IsFloatExpr), unless(isInTemplateInstantiation())) + .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(); +static bool isNarrower(const ASTContext *const Context, const QualType Lhs, + const QualType Rhs) { + assert(Lhs->isRealType()); // Either integer or floating point. + assert(Rhs->isFloatingType()); // Floating point only. + return Lhs->isIntegerType() || + (Context->getTypeSize(Rhs) > Context->getTypeSize(Lhs)); +} + +void NarrowingConversionsCheck::checkBinaryOperator( + const ASTContext *const Context, const BinaryOperator &Op) { + if (Op.getBeginLoc().isMacroID()) return; - } - const auto *Cast = Result.Nodes.getNodeAs<ImplicitCastExpr>("cast"); - if (Cast->getBeginLoc().isMacroID()) + const auto LhsType = Op.getLHS()->getType(); + const auto RhsType = Op.getRHS()->getType(); + if (isNarrower(Context, LhsType, RhsType)) + diag(Op.getOperatorLoc(), "narrowing conversion from %0 to %1") + << RhsType << LhsType; +} + +void NarrowingConversionsCheck::checkCastOperator( + const ASTContext *const Context, const ImplicitCastExpr &Cast) { + if (Cast.getBeginLoc().isMacroID()) return; - diag(Cast->getExprLoc(), "narrowing conversion from %0 to %1") - << Cast->getSubExpr()->getType() << Cast->getType(); + const auto LhsType = Cast.getType(); + const auto RhsType = Cast.getSubExpr()->getType(); + if (isNarrower(Context, LhsType, RhsType)) + diag(Cast.getExprLoc(), "narrowing conversion from %0 to %1") + << RhsType << LhsType; +} + +void NarrowingConversionsCheck::check(const MatchFinder::MatchResult &Result) { + if (const auto *Op = Result.Nodes.getNodeAs<BinaryOperator>("binary_op")) + return checkBinaryOperator(Result.Context, *Op); + if (const auto *Cast = Result.Nodes.getNodeAs<ImplicitCastExpr>("cast")) + return checkCastOperator(Result.Context, *Cast); + 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