diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h @@ -94,6 +94,10 @@ const BinaryOperator &Op); const bool WarnOnFloatingPointNarrowingConversion; + const bool WarnWithinTemplateInstantiation; + const bool WarnOnReturnOfSizeMethod; + const bool WarnOnEquivalentBitWidth; + const bool WarnOnSizeTypeAndDifferenceType; const bool PedanticMode; }; diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp @@ -8,8 +8,10 @@ #include "NarrowingConversionsCheck.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" @@ -27,12 +29,24 @@ : ClangTidyCheck(Name, Context), WarnOnFloatingPointNarrowingConversion( Options.get("WarnOnFloatingPointNarrowingConversion", true)), + WarnWithinTemplateInstantiation( + Options.get("WarnWithinTemplateInstantiation", false)), + WarnOnReturnOfSizeMethod(Options.get("WarnOnReturnOfSizeMethod", true)), + WarnOnEquivalentBitWidth(Options.get("WarnOnEquivalentBitWidth", true)), + WarnOnSizeTypeAndDifferenceType( + Options.get("WarnOnSizeTypeAndDifferenceType", true)), PedanticMode(Options.get("PedanticMode", false)) {} void NarrowingConversionsCheck::storeOptions( ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "WarnOnFloatingPointNarrowingConversion", WarnOnFloatingPointNarrowingConversion); + Options.store(Opts, "WarnWithinTemplateInstantiation", + WarnWithinTemplateInstantiation); + Options.store(Opts, "WarnOnReturnOfSizeMethod", WarnOnReturnOfSizeMethod); + Options.store(Opts, "WarnOnEquivalentBitWidth", WarnOnEquivalentBitWidth); + Options.store(Opts, "WarnOnSizeTypeAndDifferenceType", + WarnOnSizeTypeAndDifferenceType); Options.store(Opts, "PedanticMode", PedanticMode); } @@ -42,19 +56,56 @@ const auto IsCeilFloorCallExpr = expr(callExpr(callee(functionDecl( hasAnyName("::ceil", "::std::ceil", "::floor", "::std::floor"))))); + // `size()` methods typically return `size_t`, often an unsigned long. Lots of + // code uses `int` to iterate over containers and containers rarely have >2B + // elements, so we allow an option to ignore narrowings that are the result + // of the return value of a `size()` method. + // Matches a call to a method named `size()`, such as `std::vector::size`. + const auto IsSizeMethodCallExpr = + callExpr(callee(cxxMethodDecl(hasName("size")))); + + // `IsSizeMethodCallExpr` only finds narrowing calls to `size()`, not + // expressions that are promoted to `int64` due to a call to `size()`. For + // example, it will reject: `int narrowed = int_value + container.size()`. + // We attempt to address common incidents of compound expressions with + // `IsSizeMethodCallBinaryExpr`. It allows binary expressions that have one + // `size()` call argument where the other argument results in some integer + // type. + const auto IsSizeMethodCallBinaryExpr = + binaryOperator(hasOperands(IsSizeMethodCallExpr, hasType(isInteger()))); + + // Similar to the `size()` method options above, we may want to exclude other + // types from the checks, such as `size_type` and `difference_type`. These + // are often used to count elements, representented in 64 bits and assigned + // to `int`. Rarely are people counting >2B elements. + const auto IsSizeTypeOrDifferenceType = hasType( + namedDecl(anyOf(hasName("size_type"), hasName("difference_type")))); + // Casts: // i = 0.5; // void f(int); f(0.5); Finder->addMatcher( - traverse(TK_AsIs, implicitCastExpr( - hasImplicitDestinationType( - hasUnqualifiedDesugaredType(builtinType())), - hasSourceExpression(hasType( - hasUnqualifiedDesugaredType(builtinType()))), - unless(hasSourceExpression(IsCeilFloorCallExpr)), - unless(hasParent(castExpr())), - unless(isInTemplateInstantiation())) - .bind("cast")), + traverse( + TK_AsIs, + implicitCastExpr( + hasImplicitDestinationType( + hasUnqualifiedDesugaredType(builtinType())), + hasSourceExpression( + hasType(hasUnqualifiedDesugaredType(builtinType()))), + unless(hasSourceExpression(IsCeilFloorCallExpr)), + unless(hasParent(castExpr())), + WarnWithinTemplateInstantiation + ? stmt() + : stmt(unless(isInTemplateInstantiation())), + WarnOnReturnOfSizeMethod + ? castExpr() + : castExpr(unless(hasSourceExpression(anyOf( + IsSizeMethodCallExpr, IsSizeMethodCallBinaryExpr)))), + WarnOnSizeTypeAndDifferenceType + ? castExpr() + : castExpr(unless( + hasSourceExpression(IsSizeTypeOrDifferenceType)))) + .bind("cast")), this); // Binary operators: @@ -65,7 +116,16 @@ hasLHS(expr(hasType(hasUnqualifiedDesugaredType(builtinType())))), hasRHS(expr(hasType(hasUnqualifiedDesugaredType(builtinType())))), unless(hasRHS(IsCeilFloorCallExpr)), - unless(isInTemplateInstantiation()), + WarnWithinTemplateInstantiation + ? binaryOperator() + : binaryOperator(unless(isInTemplateInstantiation())), + WarnOnReturnOfSizeMethod + ? binaryOperator() + : binaryOperator(unless(hasRHS( + anyOf(IsSizeMethodCallExpr, IsSizeMethodCallBinaryExpr)))), + WarnOnSizeTypeAndDifferenceType + ? binaryOperator() + : binaryOperator(unless(hasRHS(IsSizeTypeOrDifferenceType))), // The `=` case generates an implicit cast // which is covered by the previous matcher. unless(hasOperatorName("="))) @@ -256,6 +316,16 @@ if (ToType->isUnsignedInteger()) return; const BuiltinType *FromType = getBuiltinType(Rhs); + + // 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; + } + llvm::APSInt IntegerConstant; if (getIntegerConstantExprValue(Context, Rhs, IntegerConstant)) { if (!isWideEnoughToHold(Context, IntegerConstant, *ToType)) diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst --- a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst @@ -35,6 +35,29 @@ 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:: WarnOnReturnOfSizeMethod + + When `true`, the check will warn on narrowing conversions that arise from + casts of the result of a method named 'size' (e.g. + `int n = std::vector().size();`). `true` by default. + +.. option:: WarnOnSizeTypeAndDifferenceType + + When `true`, the check will warn on narrowing conversions that arise from + casts of type names `size_type` and `difference_type`. (e.g. + `int n = std::difference(it1, it2);`). `true` 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:: PedanticMode When `true`, the check will warn on assigning a floating point constant diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-equivalentbitwidth-off.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-equivalentbitwidth-off.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-equivalentbitwidth-off.cpp @@ -0,0 +1,21 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t \ +// RUN: -config="{CheckOptions: [ \ +// RUN: {key: "cppcoreguidelines-narrowing-conversions.WarnOnEquivalentBitWidth", value: 0} \ +// RUN: ]}" \ +// RUN: -- -target x86_64-unknown-linux -fsigned-char + +void narrowing_equivalent_bitwidth_is_ok() { + int i; + unsigned int j; + i = j; + // Warning disabled with WarnOnEquivalentBitWidth=0. + j = i; + // Warning disabled with WarnOnEquivalentBitWidth=0. +} + +void most_narrowing_is_not_ok() { + int i; + long long j; + i = j; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-equivalentbitwidth-on.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-equivalentbitwidth-on.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-equivalentbitwidth-on.cpp @@ -0,0 +1,12 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t \ +// RUN: -config="{CheckOptions: [ \ +// RUN: {key: "cppcoreguidelines-narrowing-conversions.WarnOnEquivalentBitWidth", value: 1} \ +// RUN: ]}" \ +// RUN: -- -target x86_64-unknown-linux -fsigned-char + +void narrowing_equivalent_bitwidth_is_not_ok() { + int i; + unsigned int j; + i = j; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-intemplates-off.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-intemplates-off.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-intemplates-off.cpp @@ -0,0 +1,28 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t \ +// RUN: -config="{CheckOptions: [ \ +// RUN: {key: "cppcoreguidelines-narrowing-conversions.WarnWithinTemplateInstantiation", value: 0} \ +// RUN: ]}" \ +// RUN: -- -target x86_64-unknown-linux -fsigned-char + +template +void assign_in_template(OrigType jj) { + int ii; + ii = jj; +} + +void narrow_inside_template_is_ok() { + long long j = 123; + assign_in_template(j); + // Warning disabled with WarnWithinTemplateInstantiation=0. +} + +void assign_outside_template(long long jj) { + int ii; + ii = jj; + // CHECK-MESSAGES: :[[@LINE-1]]: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); +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-intemplates-on.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-intemplates-on.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-intemplates-on.cpp @@ -0,0 +1,28 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t \ +// RUN: -config="{CheckOptions: [ \ +// RUN: {key: "cppcoreguidelines-narrowing-conversions.WarnWithinTemplateInstantiation", value: 1} \ +// RUN: ]}" \ +// RUN: -- -target x86_64-unknown-linux -fsigned-char + +template +void assign_in_template(OrigType jj) { + int ii; + ii = jj; + // CHECK-MESSAGES: :[[@LINE-1]]: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: :[[@LINE-1]]: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); +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-sizemethod-off.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-sizemethod-off.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-sizemethod-off.cpp @@ -0,0 +1,43 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t \ +// RUN: -config="{CheckOptions: [ \ +// RUN: {key: "cppcoreguidelines-narrowing-conversions.WarnOnReturnOfSizeMethod", value: 0} \ +// RUN: ]}" \ +// RUN: -- -target x86_64-unknown-linux -fsigned-char + +class vector { +public: + long long size() const { return 0; } +}; + +void narrowing_size_method_is_ok() { + vector v; + int i, j; + i = v.size(); + // Warning disabled with WarnOnReturnOfSizeMethod=0. + i = j + v.size(); + // Warning disabled with WarnOnReturnOfSizeMethod=0. +} + +void narrowing_size_method_binary_expr_is_ok() { + int i; + int j; + vector v; + i = j + v.size(); + // Warning disabled with WarnOnReturnOfSizeMethod=0. +} + +void narrowing_size_method_binary_op_is_ok() { + int i, j; + vector v; + i += v.size(); + // Warning disabled with WarnOnReturnOfSizeMethod=0. + i += j + v.size(); + // Warning disabled with WarnOnReturnOfSizeMethod=0. +} + +void most_narrowing_is_not_ok() { + int i; + long long j; + i = j; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-sizemethod-on.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-sizemethod-on.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-sizemethod-on.cpp @@ -0,0 +1,32 @@ + +// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t \ +// RUN: -config="{CheckOptions: [ \ +// RUN: {key: "cppcoreguidelines-narrowing-conversions.WarnOnReturnOfSizeMethod", value: 1} \ +// RUN: ]}" \ +// RUN: -- -target x86_64-unknown-linux -fsigned-char +class vector { +public: + long long size() const { return 0; } +}; +void narrowing_size_method_is_not_ok() { + vector v; + int i, j; + i = v.size(); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] + i = j + v.size(); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] +} +void narrowing_size_method_binary_expr_is_not_ok() { + int i, j; + vector v; + i = j + v.size(); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] + i += j + v.size(); + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] +} +void narrowing_size_method_binary_op_is_not_ok() { + int i = 0; + vector v; + i += v.size(); + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-sizetypes-off.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-sizetypes-off.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-sizetypes-off.cpp @@ -0,0 +1,27 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t \ +// RUN: -config="{CheckOptions: [ \ +// RUN: {key: "cppcoreguidelines-narrowing-conversions.WarnOnSizeTypeAndDifferenceType", value: 0} \ +// RUN: ]}" \ +// RUN: -- -target x86_64-unknown-linux -fsigned-char +struct vector { + typedef long long size_type; + typedef long long difference_type; +}; +void narrowing_size_type_is_ok() { + int i; + vector::size_type j; + i = j; + // Warning disabled with WarnOnSizeTypeAndDifferenceType=0. +} +void narrowing_difference_type_is_ok() { + int i; + vector::difference_type j; + i = j; + // Warning disabled with WarnOnSizeTypeAndDifferenceType=0. +} +void most_narrowing_is_not_ok() { + int i; + long long j; + i = j; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-sizetypes-on.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-sizetypes-on.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-sizetypes-on.cpp @@ -0,0 +1,24 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t \ +// RUN: -config="{CheckOptions: [ \ +// RUN: {key: "cppcoreguidelines-narrowing-conversions.WarnOnSizeTypeAndDifferenceType", value: 1} \ +// RUN: ]}" \ +// RUN: -- -target x86_64-unknown-linux -fsigned-char + +struct vector { + typedef long long size_type; + typedef long long difference_type; +}; + +void narrowing_size_type_is_not_ok() { + int i; + vector::size_type j; + i = j; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'vector::size_type' (aka 'long long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] +} + +void narrowing_difference_type_is_not_ok() { + int i; + vector::difference_type j; + i = j; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'vector::difference_type' (aka 'long long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] +}