[clang-tools-extra] 2117613 - [clang-tidy] Allow opt-in or out of some commonly occuring patterns in NarrowingConversionsCheck.
Haojian Wu via cfe-commits
cfe-commits at lists.llvm.org
Wed May 12 11:51:37 PDT 2021
Author: Stephen Concannon
Date: 2021-05-12T20:51:25+02:00
New Revision: 211761332e4381c37edd91be7c59fc048014ff4e
URL: https://github.com/llvm/llvm-project/commit/211761332e4381c37edd91be7c59fc048014ff4e
DIFF: https://github.com/llvm/llvm-project/commit/211761332e4381c37edd91be7c59fc048014ff4e.diff
LOG: [clang-tidy] Allow opt-in or out of some commonly occuring patterns in NarrowingConversionsCheck.
Within clang-tidy's NarrowingConversionsCheck.
* Allow opt-out of some common occurring patterns, such as:
- Implicit casts between types of equivalent bit widths.
- Implicit casts occurring from the return of a ::size() method.
- Implicit casts on size_type and difference_type.
* Allow opt-in of errors within template instantiations.
This will help projects adopt these guidelines iteratively.
Developed in conjunction with Yitzhak Mandelbaum (ymandel).
Patch by Stephen Concannon!
Differential Revision: https://reviews.llvm.org/D99543
Added:
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
Modified:
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
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
index 86371fd67c533..8ce9afc8f926e 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
+++ b/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 @@ using namespace clang::ast_matchers;
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,24 @@ void NarrowingConversionsCheck::registerMatchers(MatchFinder *Finder) {
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 `
diff erence_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 `int64` due to 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 +89,13 @@ void NarrowingConversionsCheck::registerMatchers(MatchFinder *Finder) {
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 +107,12 @@ void NarrowingConversionsCheck::registerMatchers(MatchFinder *Finder) {
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("=")))
@@ -256,6 +303,16 @@ void NarrowingConversionsCheck::handleIntegralCast(const ASTContext &Context,
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/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
index 177cdabaa2527..12a6bb8eb4cec 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
@@ -94,6 +94,9 @@ class NarrowingConversionsCheck : public ClangTidyCheck {
const BinaryOperator &Op);
const bool WarnOnFloatingPointNarrowingConversion;
+ const bool WarnWithinTemplateInstantiation;
+ const bool WarnOnEquivalentBitWidth;
+ const std::string IgnoreConversionFromTypes;
const bool PedanticMode;
};
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
index 79eb5828bd186..bfc7669a4019c 100644
--- 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,26 @@ Options
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::
diff erence(it1, it2);`. The default list is empty, but one
+ suggested list for a legacy codebase would be
+ `size_t;ptr
diff _t;size_type;
diff erence_type`.
+
.. 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-option.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-equivalentbitwidth-option.cpp
new file mode 100644
index 0000000000000..947f1690d79c5
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-equivalentbitwidth-option.cpp
@@ -0,0 +1,26 @@
+// 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 j;
+ i = j;
+ // 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.
+}
+
+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-DISABLED: :[[@LINE-2]]: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-ignoreconversionfromtypes-option.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-ignoreconversionfromtypes-option.cpp
new file mode 100644
index 0000000000000..2fc5621c2fb8f
--- /dev/null
+++ b/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]
+}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-intemplates-option.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-intemplates-option.cpp
new file mode 100644
index 0000000000000..863df40ee70da
--- /dev/null
+++ b/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);
+}
More information about the cfe-commits
mailing list