Stephen updated this revision to Diff 351024. Stephen added a comment. Fix the diff.
Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103894/new/ https://reviews.llvm.org/D103894 Files: clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-equivalentbitwidth-option.cpp clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-ignoreconversionfromtypes-option.cpp clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-intemplates-option.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-intemplates-option.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-intemplates-option.cpp @@ -0,0 +1,35 @@ +// RUN: %check_clang_tidy -check-suffix=DEFAULT %s \ +// RUN: cppcoreguidelines-narrowing-conversions %t -- \ +// RUN: -config='{CheckOptions: [ \ +// RUN: ]}' + +// RUN: %check_clang_tidy -check-suffix=WARN %s \ +// RUN: cppcoreguidelines-narrowing-conversions %t -- \ +// RUN: -config='{CheckOptions: [ \ +// RUN: {key: cppcoreguidelines-narrowing-conversions.WarnWithinTemplateInstantiation, value: 1} \ +// RUN: ]}' + +template <typename OrigType> +void assign_in_template(OrigType jj) { + int ii; + ii = jj; + // DEFAULT: Warning disabled because WarnWithinTemplateInstantiation=0. + // CHECK-MESSAGES-WARN: :[[@LINE-2]]:8: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] +} + +void narrow_inside_template_not_ok() { + long long j = 123; + assign_in_template(j); +} + +void assign_outside_template(long long jj) { + int ii; + ii = jj; + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:8: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] + // CHECK-MESSAGES-WARN: :[[@LINE-2]]:8: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] +} + +void narrow_outside_template_not_ok() { + long long j = 123; + assign_outside_template(j); +} Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-ignoreconversionfromtypes-option.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-ignoreconversionfromtypes-option.cpp @@ -0,0 +1,76 @@ +// RUN: %check_clang_tidy -check-suffix=DEFAULT %s \ +// RUN: cppcoreguidelines-narrowing-conversions %t -- \ +// RUN: -config='{CheckOptions: [ \ +// RUN: ]}' + +// RUN: %check_clang_tidy -check-suffix=IGNORED %s \ +// RUN: cppcoreguidelines-narrowing-conversions %t -- \ +// RUN: -config='{CheckOptions: [ \ +// RUN: {key: cppcoreguidelines-narrowing-conversions.IgnoreConversionFromTypes, value: "global_size_t;nested_size_type"} \ +// RUN: ]}' + +// We use global_size_t instead of 'size_t' because windows predefines size_t. +typedef long long global_size_t; + +struct vector { + typedef long long nested_size_type; + + global_size_t size() const { return 0; } +}; + +void narrowing_global_size_t() { + int i; + global_size_t j; + i = j; + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:7: warning: narrowing conversion from 'global_size_t' (aka 'long long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] + // IGNORED: Warning is disabled with IgnoreConversionFromTypes=global_size_t. +} + +void narrowing_size_type() { + int i; + vector::nested_size_type j; + i = j; + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:7: warning: narrowing conversion from 'vector::nested_size_type' (aka 'long long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] + // IGNORED: Warning is disabled with IgnoreConversionFromTypes=nested_size_type. +} + +void narrowing_size_method() { + vector v; + int i, j; + i = v.size(); + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:7: warning: narrowing conversion from 'global_size_t' (aka 'long long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] + // IGNORED: Warning is disabled with IgnoreConversionFromTypes=global_size_t. + + i = j + v.size(); + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] + // IGNORED: Warning is disabled with IgnoreConversionFromTypes=global_size_t. +} + +void narrowing_size_method_binary_expr() { + int i; + int j; + vector v; + i = j + v.size(); + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] + // IGNORED: Warning is disabled with IgnoreConversionFromTypes=global_size_t. +} + +void narrowing_size_method_binary_op() { + int i, j; + vector v; + i += v.size(); + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:8: warning: narrowing conversion from 'global_size_t' (aka 'long long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] + // IGNORED: Warning is disabled with IgnoreConversionFromTypes=global_size_t. + + i += j + v.size(); + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:8: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] + // IGNORED: Warning is disabled with IgnoreConversionFromTypes=global_size_t. +} + +void most_narrowing_is_not_ok() { + int i; + long long j; + i = j; + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] + // CHECK-MESSAGES-IGNORED: :[[@LINE-2]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] +} Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-equivalentbitwidth-option.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-equivalentbitwidth-option.cpp @@ -0,0 +1,45 @@ +// RUN: %check_clang_tidy -check-suffix=DEFAULT %s \ +// RUN: cppcoreguidelines-narrowing-conversions %t -- \ +// RUN: -config='{CheckOptions: [ \ +// RUN: ]}' + +// RUN: %check_clang_tidy -check-suffix=DISABLED %s \ +// RUN: cppcoreguidelines-narrowing-conversions %t -- \ +// RUN: -config='{CheckOptions: [ \ +// RUN: {key: cppcoreguidelines-narrowing-conversions.WarnOnEquivalentBitWidth, value: 0} \ +// RUN: ]}' + +void narrowing_equivalent_bitwidth() { + int i; + unsigned int ui; + i = ui; + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:7: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] + // DISABLED: Warning disabled with WarnOnEquivalentBitWidth=0. + + float f; + i = f; + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions] + // DISABLED: Warning disabled with WarnOnEquivalentBitWidth=0. + + f = i; + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:7: warning: narrowing conversion from 'int' to 'float' [cppcoreguidelines-narrowing-conversions] + // DISABLED: Warning disabled with WarnOnEquivalentBitWidth=0. + + long long ll; + double d; + ll = d; + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:8: warning: narrowing conversion from 'double' to 'long long' [cppcoreguidelines-narrowing-conversions] + // DISABLED: Warning disabled with WarnOnEquivalentBitWidth=0. + + d = ll; + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:7: warning: narrowing conversion from 'long long' to 'double' [cppcoreguidelines-narrowing-conversions] + // DISABLED: Warning disabled with WarnOnEquivalentBitWidth=0. +} + +void most_narrowing_is_not_ok() { + int i; + long long ui; + i = ui; + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] + // CHECK-MESSAGES-DISABLED: :[[@LINE-2]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] +} Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst =================================================================== --- clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst +++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst @@ -35,6 +35,26 @@ When `true`, the check will warn on narrowing floating point conversion (e.g. ``double`` to ``float``). `true` by default. +.. option:: WarnWithinTemplateInstantiation + + When `true`, the check will warn on narrowing conversions within template + instantations. `false` by default. + +.. option:: WarnOnEquivalentBitWidth + + When `true`, the check will warn on narrowing conversions that arise from + casting between types of equivalent bit width. (e.g. + `int n = uint(0);` or `long long n = double(0);`) `true` by default. + +.. option:: IgnoreConversionFromTypes + + Narrowing conversions from any type in this semicolon-separated list will be + ignored. This may be useful to weed out commonly occurring, but less commonly + problematic assignments such as `int n = std::vector<char>().size();` or + `int n = std::difference(it1, it2);`. The default list is empty, but one + suggested list for a legacy codebase would be + `size_t;ptrdiff_t;size_type;difference_type`. + .. option:: PedanticMode When `true`, the check will warn on assigning a floating point constant Index: clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h =================================================================== --- clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h +++ clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h @@ -93,7 +93,14 @@ void handleBinaryOperator(const ASTContext &Context, const BinaryOperator &Op); + bool isWarningInhibitedByEquivalentSize(const ASTContext &Context, + const BuiltinType &FromType, + const BuiltinType &ToType) const; + const bool WarnOnFloatingPointNarrowingConversion; + const bool WarnWithinTemplateInstantiation; + const bool WarnOnEquivalentBitWidth; + const std::string IgnoreConversionFromTypes; const bool PedanticMode; }; Index: clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp =================================================================== --- clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp +++ clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp @@ -7,9 +7,12 @@ //===----------------------------------------------------------------------===// #include "NarrowingConversionsCheck.h" +#include "../utils/OptionsUtils.h" #include "clang/AST/ASTContext.h" +#include "clang/AST/Expr.h" #include "clang/AST/Type.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" #include "llvm/ADT/APSInt.h" #include "llvm/ADT/SmallString.h" #include "llvm/ADT/SmallVector.h" @@ -21,18 +24,33 @@ namespace clang { namespace tidy { namespace cppcoreguidelines { +namespace { +auto hasAnyListedName(const std::string &Names) { + const std::vector<std::string> NameList = + utils::options::parseStringList(Names); + return hasAnyName(std::vector<StringRef>(NameList.begin(), NameList.end())); +} +} // namespace NarrowingConversionsCheck::NarrowingConversionsCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), WarnOnFloatingPointNarrowingConversion( Options.get("WarnOnFloatingPointNarrowingConversion", true)), + WarnWithinTemplateInstantiation( + Options.get("WarnWithinTemplateInstantiation", false)), + WarnOnEquivalentBitWidth(Options.get("WarnOnEquivalentBitWidth", true)), + IgnoreConversionFromTypes(Options.get("IgnoreConversionFromTypes", "")), PedanticMode(Options.get("PedanticMode", false)) {} void NarrowingConversionsCheck::storeOptions( ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "WarnOnFloatingPointNarrowingConversion", WarnOnFloatingPointNarrowingConversion); + Options.store(Opts, "WarnWithinTemplateInstantiation", + WarnWithinTemplateInstantiation); + Options.store(Opts, "WarnOnEquivalentBitWidth", WarnOnEquivalentBitWidth); + Options.store(Opts, "IgnoreConversionFromTypes", IgnoreConversionFromTypes); Options.store(Opts, "PedanticMode", PedanticMode); } @@ -42,6 +60,25 @@ const auto IsCeilFloorCallExpr = expr(callExpr(callee(functionDecl( hasAnyName("::ceil", "::std::ceil", "::floor", "::std::floor"))))); + // We may want to exclude other types from the checks, such as `size_type` + // and `difference_type`. These are often used to count elements, represented + // in 64 bits and assigned to `int`. Rarely are people counting >2B elements. + const auto IsConversionFromIgnoredType = + hasType(namedDecl(hasAnyListedName(IgnoreConversionFromTypes))); + + // `IsConversionFromIgnoredType` will ignore narrowing calls from those types, + // but not expressions that are promoted to an ignored type as a result of a + // binary expression with one of those types. + // For example, it will continue to reject: + // `int narrowed = int_value + container.size()`. + // We attempt to address common incidents of compound expressions with + // `IsIgnoredTypeTwoLevelsDeep`, allowing binary expressions that have one + // operand of the ignored types and the other operand of another integer type. + const auto IsIgnoredTypeTwoLevelsDeep = + anyOf(IsConversionFromIgnoredType, + binaryOperator(hasOperands(IsConversionFromIgnoredType, + hasType(isInteger())))); + // Casts: // i = 0.5; // void f(int); f(0.5); @@ -53,7 +90,13 @@ hasUnqualifiedDesugaredType(builtinType()))), unless(hasSourceExpression(IsCeilFloorCallExpr)), unless(hasParent(castExpr())), - unless(isInTemplateInstantiation())) + WarnWithinTemplateInstantiation + ? stmt() + : stmt(unless(isInTemplateInstantiation())), + IgnoreConversionFromTypes.empty() + ? castExpr() + : castExpr(unless(hasSourceExpression( + IsIgnoredTypeTwoLevelsDeep)))) .bind("cast")), this); @@ -65,7 +108,12 @@ hasLHS(expr(hasType(hasUnqualifiedDesugaredType(builtinType())))), hasRHS(expr(hasType(hasUnqualifiedDesugaredType(builtinType())))), unless(hasRHS(IsCeilFloorCallExpr)), - unless(isInTemplateInstantiation()), + WarnWithinTemplateInstantiation + ? binaryOperator() + : binaryOperator(unless(isInTemplateInstantiation())), + IgnoreConversionFromTypes.empty() + ? binaryOperator() + : binaryOperator(unless(hasRHS(IsIgnoredTypeTwoLevelsDeep))), // The `=` case generates an implicit cast // which is covered by the previous matcher. unless(hasOperatorName("="))) @@ -170,6 +218,22 @@ return ToIntegerRange.contains(IntegerConstant); } +// Returns true iff the floating point constant can be losslessly represented +// by an integer in the given destination type. eg. 2.0 can be accurately +// represented by an int32_t, but neither 2^33 nor 2.001 can. +static bool isFloatExactlyRepresentable(const ASTContext &Context, + const llvm::APFloat &FloatConstant, + const QualType &DestType) { + unsigned DestWidth = Context.getIntWidth(DestType); + bool DestSigned = DestType->isSignedIntegerOrEnumerationType(); + llvm::APSInt Result = llvm::APSInt(DestWidth, !DestSigned); + bool IsExact = false; + bool Overflows = FloatConstant.convertToInteger( + Result, llvm::APFloat::rmTowardZero, &IsExact) & + llvm::APFloat::opInvalidOp; + return !Overflows && IsExact; +} + static llvm::SmallString<64> getValueAsString(const llvm::APSInt &Value, uint64_t HexBits) { llvm::SmallString<64> Str; @@ -186,6 +250,21 @@ return Str; } +bool NarrowingConversionsCheck::isWarningInhibitedByEquivalentSize( + const ASTContext &Context, const BuiltinType &FromType, + const BuiltinType &ToType) const { + // With this option, we don't warn on conversions that have equivalent width + // in bits. eg. uint32 <-> int32. + if (!WarnOnEquivalentBitWidth) { + uint64_t FromTypeSize = Context.getTypeSize(&FromType); + uint64_t ToTypeSize = Context.getTypeSize(&ToType); + if (FromTypeSize == ToTypeSize) { + return true; + } + } + return false; +} + void NarrowingConversionsCheck::diagNarrowType(SourceLocation SourceLoc, const Expr &Lhs, const Expr &Rhs) { @@ -256,6 +335,8 @@ if (ToType->isUnsignedInteger()) return; const BuiltinType *FromType = getBuiltinType(Rhs); + + llvm::APSInt IntegerConstant; if (getIntegerConstantExprValue(Context, Rhs, IntegerConstant)) { if (!isWideEnoughToHold(Context, IntegerConstant, *ToType)) @@ -263,6 +344,9 @@ Context.getTypeSize(FromType)); return; } + + if (isWarningInhibitedByEquivalentSize(Context, *FromType, *ToType)) + return; if (!isWideEnoughToHold(Context, *FromType, *ToType)) diagNarrowTypeToSignedInt(SourceLoc, Lhs, Rhs); } @@ -287,7 +371,10 @@ diagNarrowIntegerConstant(SourceLoc, Lhs, Rhs, IntegerConstant); return; } + const BuiltinType *FromType = getBuiltinType(Rhs); + if (isWarningInhibitedByEquivalentSize(Context, *FromType, *ToType)) + return; if (!isWideEnoughToHold(Context, *FromType, *ToType)) diagNarrowType(SourceLoc, Lhs, Rhs); } @@ -296,25 +383,21 @@ const ASTContext &Context, SourceLocation SourceLoc, const Expr &Lhs, const Expr &Rhs) { llvm::APFloat FloatConstant(0.0); + if (getFloatingConstantExprValue(Context, Rhs, FloatConstant)) { + if (!isFloatExactlyRepresentable(Context, FloatConstant, Lhs.getType())) + return diagNarrowConstant(SourceLoc, Lhs, Rhs); - // We always warn when Rhs is non-constexpr. - if (!getFloatingConstantExprValue(Context, Rhs, FloatConstant)) - return diagNarrowType(SourceLoc, Lhs, Rhs); + if (PedanticMode) + return diagConstantCast(SourceLoc, Lhs, Rhs); - QualType DestType = Lhs.getType(); - unsigned DestWidth = Context.getIntWidth(DestType); - bool DestSigned = DestType->isSignedIntegerOrEnumerationType(); - llvm::APSInt Result = llvm::APSInt(DestWidth, !DestSigned); - bool IsExact = false; - bool Overflows = FloatConstant.convertToInteger( - Result, llvm::APFloat::rmTowardZero, &IsExact) & - llvm::APFloat::opInvalidOp; - // We warn iff the constant floating point value is not exactly representable. - if (Overflows || !IsExact) - return diagNarrowConstant(SourceLoc, Lhs, Rhs); + return; + } - if (PedanticMode) - return diagConstantCast(SourceLoc, Lhs, Rhs); + const BuiltinType *FromType = getBuiltinType(Rhs); + const BuiltinType *ToType = getBuiltinType(Lhs); + if (isWarningInhibitedByEquivalentSize(Context, *FromType, *ToType)) + return; + diagNarrowType(SourceLoc, Lhs, Rhs); // Assumed always lossy. } void NarrowingConversionsCheck::handleFloatingToBoolean(
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits