whisperity created this revision.
whisperity added reviewers: aaron.ballman, alexfh, hokein, JonasToth.
whisperity added projects: clang, clang-tools-extra.
Herald added subscribers: gamesh411, Szelethus, rnkovacs, xazax.hun.
whisperity requested review of this revision.
While the original check's (D69560 <https://reviews.llvm.org/D69560>) purpose
is to identify potentially dangerous functions based on the parameter types (as
identifier names do not mean //anything// when it comes to the language rules),
unfortunately, such a plain interface check rule can be incredibly noisy. While
the previous //"filtering heuristic"// in D78652
<https://reviews.llvm.org/D78652> is able to find many similar usages, there is
an entire class of parameters that should not be warned about very easily mixed
by that check: parameters that have a name and their name follows a pattern,
e.g. `text1, text2, text3, ...`. [1]
This patch implements a simple, but powerful rule, that allows us to detect
such cases and ensure that no warnings are emitted for parameter sequences that
follow a pattern, even if their types allow for them to be potentially mixed at
a call site.
Given a threshold `k`, warnings about two parameters are filtered from the
result set if the names of the parameters are either prefixes or suffixes of
each other, with at most `k` letters difference on the non-common end.
(Assuming that the names themselves are at least `k` long.)
The above `text1, text2` is an example of this. (Live finding from Xerces.)
`LHS` and `RHS` are also fitting the bill here. (Live finding from... virtually
any project.)
So does `Qmat, Tmat, Rmat`. (Live finding from I think OpenCV.)
On average, **15%** reduction in findings were achieved with this feature
turned on.
Similarly to the other filtering patch on the "relatedness modelling", this
option is implemented as a check option,
`NamePrefixSuffixSilenceDissimilarityTreshold`. If `0`, the heuristic is turned
off. **Defaults to `1`**, i.e. a single letter difference ("pattern") on either
end of the parameter name suffice for silence. This is the most reasonable
default.
----
[1] It has been identified before by other research that such patterned names
carry no viable information and such names are useless for a name-based
analysis where argument vs. parameter names are contrasted with one another.
[Rice2017]
//[Rice2017]//: Andrew Rice, et al.: Detecting argument selection defects. In:
Proceedings of the ACM on Programming Languages, 1, pp. 104:1-104:23, 2017.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D97297
Files:
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-prefixsuffixname.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
@@ -2,7 +2,8 @@
// RUN: -config='{CheckOptions: [ \
// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
-// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "bool;MyBool;struct U;MAKE_LOGICAL_TYPE(int)"} \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "bool;MyBool;struct U;MAKE_LOGICAL_TYPE(int)"}, \
+// RUN: {key: bugprone-easily-swappable-parameters.NamePrefixSuffixSilenceDissimilarityTreshold, value: 0} \
// RUN: ]}' -- -x c
#define bool _Bool
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-prefixsuffixname.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-prefixsuffixname.cpp
@@ -0,0 +1,53 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN: -config='{CheckOptions: [ \
+// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.NamePrefixSuffixSilenceDissimilarityTreshold, value: 1} \
+// RUN: ]}' --
+
+namespace std {
+struct string {};
+} // namespace std
+class Matrix {};
+
+void test1(int Foo, int Bar) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 2 adjacent parameters of 'test1' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:16: note: the first parameter in the range is 'Foo'
+// CHECK-MESSAGES: :[[@LINE-3]]:25: note: the last parameter in the range is 'Bar'
+
+void test2(int A, int B) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 2 adjacent parameters of 'test2' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-2]]:16: note: the first parameter in the range is 'A'
+// CHECK-MESSAGES: :[[@LINE-3]]:23: note: the last parameter in the range is 'B'
+
+void test3(int Val1, int Val2) {} // NO-WARN.
+
+void test4(int ValA, int Valb) {} // NO-WARN.
+
+void test5(int Val1, int ValZ) {} // NO-WARN.
+
+void test6(int PValue, int QValue) {} // NO-WARN.
+
+void test7(std::string Astr, std::string Bstr) {} // NO-WARN.
+
+void test8(int Aladdin, int Alabaster) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 2 adjacent parameters of 'test8' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-2]]:16: note: the first parameter in the range is 'Aladdin'
+// CHECK-MESSAGES: :[[@LINE-3]]:29: note: the last parameter in the range is 'Alabaster'
+
+void test9(Matrix Qmat, Matrix Rmat, Matrix Tmat) {} // NO-WARN.
+
+void test10(int Something, int Other, int Foo, int Bar1, int Bar2, int Baz, int Qux) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 4 adjacent parameters of 'test10' of similar type ('int') are
+// CHECK-MESSAGES: :[[@LINE-2]]:17: note: the first parameter in the range is 'Something'
+// CHECK-MESSAGES: :[[@LINE-3]]:52: note: the last parameter in the range is 'Bar1'
+//
+// CHECK-MESSAGES: :[[@LINE-5]]:58: warning: 3 adjacent parameters of 'test10' of similar type ('int') are
+// CHECK-MESSAGES: :[[@LINE-6]]:62: note: the first parameter in the range is 'Bar2'
+// CHECK-MESSAGES: :[[@LINE-7]]:81: note: the last parameter in the range is 'Qux'
+
+void test11(int Foobar, int Foo) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 2 adjacent parameters of 'test11' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-2]]:17: note: the first parameter in the range is 'Foobar'
+// CHECK-MESSAGES: :[[@LINE-3]]:29: note: the last parameter in the range is 'Foo'
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
@@ -2,7 +2,8 @@
// RUN: -config='{CheckOptions: [ \
// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 3}, \
// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
-// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""} \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.NamePrefixSuffixSilenceDissimilarityTreshold, value: 0} \
// RUN: ]}' --
int add(int Left, int Right) { return Left + Right; } // NO-WARN: Only 2 parameters.
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
@@ -2,7 +2,8 @@
// RUN: -config='{CheckOptions: [ \
// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
-// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""} \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.NamePrefixSuffixSilenceDissimilarityTreshold, value: 0} \
// RUN: ]}' --
#define assert(X) ((void)(X))
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
@@ -2,7 +2,8 @@
// RUN: -config='{CheckOptions: [ \
// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: "\"\";Foo;Bar"}, \
-// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "T"} \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "T"}, \
+// RUN: {key: bugprone-easily-swappable-parameters.NamePrefixSuffixSilenceDissimilarityTreshold, value: 0} \
// RUN: ]}' --
void ignoredUnnamed(int I, int, int) {} // NO-WARN: No >= 2 length of non-unnamed.
Index: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
+++ clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
@@ -38,6 +38,12 @@
/// The parameter typename suffixes (as written in the source code) to be
/// ignored.
const std::vector<std::string> IgnoredParameterTypeSuffixes;
+
+ /// The number of characters two parameter names might be dissimilar at
+ /// either end for the report about the parameters to be silenced.
+ /// E.g. the names "LHS" and "RHS" are 1-dissimilar suffixes of each other,
+ /// while "Text1" and "Text2" are 1-dissimilar prefixes of each other.
+ const std::size_t NamePrefixSuffixSilenceDissimilarityTreshold;
};
} // namespace bugprone
Index: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
@@ -18,7 +18,7 @@
namespace optutils = clang::tidy::utils::options;
/// The default value for the MinimumLength check option.
-static constexpr unsigned DefaultMinimumLength = 2;
+static constexpr std::size_t DefaultMinimumLength = 2;
/// The default value for ignored parameter names.
static const std::string DefaultIgnoredParameterNames =
@@ -53,6 +53,11 @@
"reverse_iterator",
"reverse_const_iterator"});
+/// The default value for the NamePrefixSuffixSilenceDissimilarityTreshold
+/// check option.
+static constexpr std::size_t
+ DefaultNamePrefixSuffixSilenceDissimilarityTreshold = 1;
+
#ifndef NDEBUG
template <std::size_t Width>
@@ -93,6 +98,8 @@
namespace filter {
static bool isIgnoredParameter(const TheCheck &Check, const ParmVarDecl *Node);
+static bool prefixSuffixCoverUnderThreshold(std::size_t Threshold,
+ StringRef Str1, StringRef Str2);
} // namespace filter
namespace model {
@@ -222,13 +229,25 @@
for (std::size_t I = StartIndex + 1; I < NumParams; ++I) {
const ParmVarDecl *Ith = FD->getParamDecl(I);
- LLVM_DEBUG(llvm::dbgs() << "Check param #" << I << "...\n");
-
+ StringRef ParamName = Ith->getName();
+ LLVM_DEBUG(llvm::dbgs()
+ << "Check param #" << I << " '" << ParamName << "'...\n");
if (filter::isIgnoredParameter(Check, Ith)) {
LLVM_DEBUG(llvm::dbgs() << "Param #" << I << " is ignored. Break!\n");
break;
}
+ StringRef PrevParamName = FD->getParamDecl(I - 1)->getName();
+ if (!ParamName.empty() && !PrevParamName.empty() &&
+ filter::prefixSuffixCoverUnderThreshold(
+ Check.NamePrefixSuffixSilenceDissimilarityTreshold, PrevParamName,
+ ParamName)) {
+ LLVM_DEBUG(llvm::dbgs() << "Parameter '" << ParamName
+ << "' follows a pattern with previous parameter '"
+ << PrevParamName << "'. Break!\n");
+ break;
+ }
+
// Now try to go forward and build the range of [Start, ..., I, I + 1, ...]
// parameters that can be messed up at a call site.
MixableParameterRange::MixVector MixesOfIth;
@@ -329,6 +348,60 @@
return false;
}
+
+static void padStringAtEnd(SmallVectorImpl<char> &Str, std::size_t ToLen) {
+ while (Str.size() < ToLen)
+ Str.emplace_back('\0');
+}
+
+static void padStringAtBegin(SmallVectorImpl<char> &Str, std::size_t ToLen) {
+ while (Str.size() < ToLen)
+ Str.insert(Str.begin(), '\0');
+}
+
+static bool isCommonPrefixWithoutSomeCharacters(std::size_t N, StringRef S1,
+ StringRef S2) {
+ StringRef S1Prefix = S1.take_front(S1.size() - N),
+ S2Prefix = S2.take_front(S2.size() - N);
+ return S1Prefix == S2Prefix && !S1Prefix.empty();
+}
+
+static bool isCommonSuffixWithoutSomeCharacters(std::size_t N, StringRef S1,
+ StringRef S2) {
+ StringRef S1Suffix = S1.take_back(S1.size() - N),
+ S2Suffix = S2.take_back(S2.size() - N);
+ return S1Suffix == S2Suffix && !S1Suffix.empty();
+}
+
+/// Returns whether the two strings are prefixes or suffixes of each other with
+/// at most Threshold characters differing on the non-common end.
+static bool prefixSuffixCoverUnderThreshold(std::size_t Threshold,
+ StringRef Str1, StringRef Str2) {
+ if (Threshold == 0)
+ return false;
+
+ // Pad the two strings to the longer length.
+ std::size_t BiggerLength = std::max(Str1.size(), Str2.size());
+ SmallString<32> S1PadE{Str1}, S2PadE{Str2};
+ padStringAtEnd(S1PadE, BiggerLength);
+ padStringAtEnd(S2PadE, BiggerLength);
+
+ if (isCommonPrefixWithoutSomeCharacters(
+ Threshold, StringRef{S1PadE.begin(), BiggerLength},
+ StringRef{S2PadE.begin(), BiggerLength}))
+ return true;
+
+ SmallString<32> S1PadB{Str1}, S2PadB{Str2};
+ padStringAtBegin(S1PadB, BiggerLength);
+ padStringAtBegin(S2PadB, BiggerLength);
+
+ if (isCommonSuffixWithoutSomeCharacters(
+ Threshold, StringRef{S1PadB.begin(), BiggerLength},
+ StringRef{S2PadB.begin(), BiggerLength}))
+ return true;
+
+ return false;
+}
} // namespace filter
/// Matches functions that have at least the specified amount of parameters.
@@ -384,7 +457,10 @@
Options.get("IgnoredParameterNames", DefaultIgnoredParameterNames))),
IgnoredParameterTypeSuffixes(optutils::parseStringList(
Options.get("IgnoredParameterTypeSuffixes",
- DefaultIgnoredParameterTypeSuffixes))) {}
+ DefaultIgnoredParameterTypeSuffixes))),
+ NamePrefixSuffixSilenceDissimilarityTreshold(
+ Options.get("NamePrefixSuffixSilenceDissimilarityTreshold",
+ DefaultNamePrefixSuffixSilenceDissimilarityTreshold)) {}
void EasilySwappableParametersCheck::storeOptions(
ClangTidyOptions::OptionMap &Opts) {
@@ -393,6 +469,8 @@
optutils::serializeStringList(IgnoredParameterNames));
Options.store(Opts, "IgnoredParameterTypeSuffixes",
optutils::serializeStringList(IgnoredParameterTypeSuffixes));
+ Options.store(Opts, "NamePrefixSuffixSilenceDissimilarityTreshold",
+ NamePrefixSuffixSilenceDissimilarityTreshold);
}
void EasilySwappableParametersCheck::registerMatchers(MatchFinder *Finder) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
