ccotter updated this revision to Diff 530349.
ccotter added a comment.
- Exclude false positive
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143971/new/
https://reviews.llvm.org/D143971
Files:
clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone/string-constructor.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp
@@ -5,12 +5,13 @@
class allocator {};
template <typename T>
class char_traits {};
-template <typename C, typename T = std::char_traits<C>, typename A = std::allocator<C> >
+template <typename C, typename T = std::char_traits<C>, typename A = std::allocator<C>>
struct basic_string {
basic_string();
- basic_string(const C*, unsigned int size);
- basic_string(const C *, const A &allocator = A());
- basic_string(unsigned int size, C c);
+ basic_string(const basic_string&, unsigned int, unsigned int, const A & = A());
+ basic_string(const C*, unsigned int);
+ basic_string(const C*, const A& = A());
+ basic_string(unsigned int, C);
};
typedef basic_string<char> string;
typedef basic_string<wchar_t> wstring;
@@ -18,7 +19,7 @@
template <typename C, typename T = std::char_traits<C>>
struct basic_string_view {
basic_string_view();
- basic_string_view(const C *, unsigned int size);
+ basic_string_view(const C *, unsigned int);
basic_string_view(const C *);
};
typedef basic_string_view<char> string_view;
@@ -28,20 +29,76 @@
const char* kText = "";
const char kText2[] = "";
extern const char kText3[];
+char getChar();
+const char* getCharPtr();
+
+struct CharLikeObj {
+ operator char() const;
+};
+
+unsigned char getUChar();
void Test() {
- std::string str('x', 4);
- // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor parameters are probably swapped; expecting string(count, character) [bugprone-string-constructor]
- // CHECK-FIXES: std::string str(4, 'x');
- std::wstring wstr(L'x', 4);
- // CHECK-MESSAGES: [[@LINE-1]]:16: warning: string constructor parameters are probably swapped
- // CHECK-FIXES: std::wstring wstr(4, L'x');
+ short sh;
+ int i;
+ int& ref_i = i;
+ char ch;
+ CharLikeObj char_like_obj;
+
+ std::string swapped('x', 4);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped(4, 'x');
+ std::wstring wswapped(L'x', 4);
+ // CHECK-MESSAGES: [[@LINE-1]]:16: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::wstring wswapped(4, L'x');
+ std::string swapped2('x', i);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped2(i, 'x');
+ std::string swapped3(ch, 4);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped3(4, ch);
+ std::string swapped4(ch, i);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped4(i, ch);
+ std::string swapped5('x', (int)'x');
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped5((int)'x', 'x');
+ std::string swapped6(getChar(), 10);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped6(10, getChar());
+ std::string swapped7((('x')), ( i ));
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped7(( i ), (('x')));
+ std::string swapped8((ch), (i));
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped8((i), (ch));
+ std::string swapped9((getChar()), (i));
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped9((i), (getChar()));
std::string s0(0, 'x');
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: constructor creating an empty string
std::string s1(-4, 'x');
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: negative value used as length parameter
std::string s2(0x1000000, 'x');
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: suspicious large length parameter
+ std::string s3(kText[0], sh);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: confusing string fill constructor arguments;
+ // CHECK-FIXES: std::string s3(kText[0], sh);
+ std::string s4(kText[1], 10);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: confusing string fill constructor arguments;
+ // CHECK-FIXES: std::string s4(kText[1], 10);
+ std::string s5(kText[1], i);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: confusing string fill constructor arguments;
+ // CHECK-FIXES: std::string s5(kText[1], i);
+ std::string s6(kText[1], ref_i);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: confusing string fill constructor arguments;
+ // CHECK-FIXES: std::string s6(kText[1], ref_i);
+ std::string s7(*getCharPtr(), 10);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: confusing string fill constructor arguments;
+ // CHECK-FIXES: std::string s7(*getCharPtr(), 10);
+ std::string s8(char_like_obj, 10);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string s8(10, char_like_obj);
std::string q0("test", 0);
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: constructor creating an empty string
@@ -91,12 +148,19 @@
}
void Valid() {
+ int i;
std::string empty();
std::string str(4, 'x');
std::wstring wstr(4, L'x');
std::string s1("test", 4);
std::string s2("test", 3);
std::string s3("test");
+ std::string s4((int)kText[1], i);
+ std::string s5(kText[1], (char)i);
+
+ unsigned char Sz = 5;
+ std::string s6(Sz, 'x');
+ std::string s7(getUChar(), 'x');
std::string_view emptyv();
std::string_view sv1("test", 4);
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/string-constructor.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/bugprone/string-constructor.rst
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/string-constructor.rst
@@ -5,13 +5,24 @@
Finds string constructors that are suspicious and probably errors.
-A common mistake is to swap parameters to the 'fill' string-constructor.
+A common mistake is to swap arguments to the 'fill' string-constructor.
Examples:
.. code-block:: c++
+ int x;
std::string str('x', 50); // should be str(50, 'x')
+ std::string str2('a', x); // should be str2(x, 'a')
+
+Constructing a string using the 'fill' constructor where both arguments
+are implicitly cast is confusing and likely indicative of a bug.
+
+.. code-block: c++
+
+ char buf[10];
+ std::string str(buf[1], 5); // First arg should be '&buf[1]'?
+ std::string str2((int)buf[1], 5); // Ok - explicitly cast to express intent
Calling the string-literal constructor with a length bigger than the literal is
suspicious and adds extra random characters to the string.
@@ -23,7 +34,7 @@
std::string("test", 200); // Will include random characters after "test".
std::string_view("test", 200);
-Creating an empty string from constructors with parameters is considered
+Creating an empty string from constructors with arguments is considered
suspicious. The programmer should use the empty constructor instead.
Examples:
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -219,6 +219,10 @@
<clang-tidy/checks/bugprone/incorrect-roundings>` check by adding support for
other floating point representations in float constant like ``0.5L``.
+- Updated :doc:`bugprone-string-constructor
+ <clang-tidy/checks/bugprone/string-constructor>` check to flag more potentially
+ buggy code.
+
- Deprecated check-local options `HeaderFileExtensions` and `ImplementationFileExtensions`
in :doc:`bugprone-suspicious-include
<clang-tidy/checks/bugprone/suspicious-include>` check.
Index: clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp
@@ -48,6 +48,17 @@
StringNames(utils::options::parseStringList(
Options.get("StringNames", DefaultStringNames))) {}
+namespace {
+AST_MATCHER_P2(CXXConstructExpr, hasRawArgument, unsigned, N,
+ ast_matchers::internal::Matcher<Expr>, InnerMatcher) {
+ if (N >= Node.getNumArgs()) {
+ return false;
+ }
+ const Expr *Arg = Node.getArg(N);
+ return InnerMatcher.matches(*Arg, Finder, Builder);
+}
+} // namespace
+
void StringConstructorCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "WarnOnLargeLength", WarnOnLargeLength);
Options.store(Opts, "LargeLengthThreshold", LargeLengthThreshold);
@@ -55,8 +66,13 @@
}
void StringConstructorCheck::registerMatchers(MatchFinder *Finder) {
+ const auto SignedCharType = qualType(isAnyCharacter(), isSignedInteger());
const auto ZeroExpr = expr(ignoringParenImpCasts(integerLiteral(equals(0))));
- const auto CharExpr = expr(ignoringParenImpCasts(characterLiteral()));
+ const auto CharExpr =
+ expr(anyOf(ignoringParenImpCasts(characterLiteral()),
+ declRefExpr(hasDeclaration(varDecl(hasType(SignedCharType)))),
+ ignoringParens(callExpr(callee(
+ functionDecl(returns(qualType(SignedCharType))))))));
const auto NegativeExpr = expr(ignoringParenImpCasts(
unaryOperator(hasOperatorName("-"),
hasUnaryOperand(integerLiteral(unless(equals(0)))))));
@@ -76,6 +92,14 @@
const auto ConstStrLiteral = expr(ignoringParenImpCasts(anyOf(
BoundStringLiteral, declRefExpr(hasDeclaration(anyOf(
ConstPtrStrLiteralDecl, ConstStrLiteralDecl))))));
+ const auto NonCharacterInteger =
+ qualType(isInteger(), unless(isAnyCharacter()));
+ const auto CharToIntCastExpr = implicitCastExpr(
+ hasSourceExpression(expr(hasType(qualType(isAnyCharacter())))),
+ hasImplicitDestinationType(NonCharacterInteger));
+ const auto IntToCharCastExpr =
+ implicitCastExpr(hasSourceExpression(expr(hasType(NonCharacterInteger))),
+ hasImplicitDestinationType(qualType(isAnyCharacter())));
// Check the fill constructor. Fills the string with n consecutive copies of
// character c. [i.e string(size_t n, char c);].
@@ -84,15 +108,24 @@
hasDeclaration(cxxMethodDecl(hasName("basic_string"))),
hasArgument(0, hasType(qualType(isInteger()))),
hasArgument(1, hasType(qualType(isInteger()))),
- anyOf(
- // Detect the expression: string('x', 40);
- hasArgument(0, CharExpr.bind("swapped-parameter")),
- // Detect the expression: string(0, ...);
- hasArgument(0, ZeroExpr.bind("empty-string")),
- // Detect the expression: string(-4, ...);
- hasArgument(0, NegativeExpr.bind("negative-length")),
- // Detect the expression: string(0x1234567, ...);
- hasArgument(0, LargeLengthExpr.bind("large-length"))))
+ anyOf(anyOf(
+ // Detect the expression: string('x', 40);
+ hasArgument(0, CharExpr.bind("swapped-parameter")),
+ // Detect the expression: string(0, ...);
+ hasArgument(0, ZeroExpr.bind("empty-string")),
+ // Detect the expression: string(-4, ...);
+ hasArgument(0, NegativeExpr.bind("negative-length")),
+ // Detect the expression: string(0x1234567, ...);
+ hasArgument(0, LargeLengthExpr.bind("large-length"))),
+ // Finally, check for implicitly cast of both arguments. When
+ // the source type and destination types of the implicit casts
+ // from character to integer or vice versa for both arguments,
+ // this can be confusing to readers and indicative of a bug in
+ // the program. E.g., string(ch, i) implicitly casts 'ch' to
+ // size_type and casts 'i' to char.
+ allOf(hasRawArgument(0, CharToIntCastExpr),
+ hasRawArgument(1, IntToCharCastExpr.bind(
+ "implicit-cast-both-args")))))
.bind("constructor"),
this);
@@ -147,7 +180,7 @@
if (Result.Nodes.getNodeAs<Expr>("swapped-parameter")) {
const Expr *P0 = E->getArg(0);
const Expr *P1 = E->getArg(1);
- diag(Loc, "string constructor parameters are probably swapped;"
+ diag(Loc, "string constructor arguments are probably swapped;"
" expecting string(count, character)")
<< tooling::fixit::createReplacement(*P0, *P1, Ctx)
<< tooling::fixit::createReplacement(*P1, *P0, Ctx);
@@ -178,6 +211,13 @@
}
diag(Loc, "constructing string from nullptr is undefined behaviour");
}
+ } else if (const auto *E =
+ Result.Nodes.getNodeAs<Expr>("implicit-cast-both-args")) {
+ diag(Loc, "confusing string fill constructor arguments;"
+ " calling as string(count, character) due to implicit casting of "
+ "both arguments;"
+ " use explicit casts if string(count, character) is the intended "
+ "constructor");
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
