gchatelet updated this revision to Diff 171692.
gchatelet added a comment.
- Add more checks, disable bool implicit casts warning, enable ternary operator
warnings.
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,129 @@
// 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 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_ok() {
+ int i;
+ while (i) {
+ }
+ for (; i;) {
+ }
+ if (i) {
+ }
+}
+
+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 +213,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;``.
+A narrowing conversion is currently defined as a conversion 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,7 +9,12 @@
#include "NarrowingConversionsCheck.h"
#include "clang/AST/ASTContext.h"
+#include "clang/AST/Type.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/APInt.h"
+#include "llvm/ADT/APSInt.h"
+#include "llvm/Support/Mutex.h"
+#include <cstdint>
using namespace clang::ast_matchers;
@@ -21,48 +26,175 @@
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));
+ const auto IsBuiltinOrEnumType = anyOf(builtinType(), enumType());
- // 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;
+}
+
+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;
+};
+
+static IntegerRange createFromType(const ASTContext &Context,
+ const BuiltinType *T) {
+ assert(T && "T must not be nullptr");
+ if (T->isFloatingPoint()) {
+ const 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]
+
+ // 'One' is created with PrecisionBits plus two bytes:
+ // - One to express the missing negative value of 2's complement
+ // representation.
+ // - One for the sign
+ llvm::APSInt Tmp(PrecisionBits + 2, false);
+ Tmp = 1UL;
+ llvm::APSInt UpperValue = Tmp << PrecisionBits;
+ llvm::APSInt LowerValue = UpperValue;
+ LowerValue.negate();
+ return {LowerValue, UpperValue};
+ }
+ 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->getScalarTypeKind() == clang::Type::STK_Bool
+ ? false
+ : ToType->getKind() < FromType->getKind();
+
+ if (FromType->isRealFloatingType() && ToType->isRealFloatingType())
+ return ToType->getKind() < FromType->getKind();
+
+ const auto FromIntegerRange = createFromType(Context, FromType);
+ const auto 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) {
+ const QualType LhsType = Lhs->getType();
+ const 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;
+ const auto 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)) {
+ const bool NarrowLhs = diagIfNarrowConstant(
+ Context, CO->getLHS()->getExprLoc(), Lhs, CO->getLHS());
+ const bool NarrowRhs = diagIfNarrowConstant(
+ Context, CO->getRHS()->getExprLoc(), Lhs, CO->getRHS());
+ return NarrowLhs || NarrowRhs;
}
- const auto *Cast = Result.Nodes.getNodeAs<ImplicitCastExpr>("cast");
- if (Cast->getBeginLoc().isMacroID())
+ 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
