diff --git a/clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp --- a/clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp +++ b/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, 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); @@ -56,7 +67,11 @@ void StringConstructorCheck::registerMatchers(MatchFinder *Finder) { 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(qualType(isAnyCharacter()))))), + ignoringParens(callExpr( + callee(functionDecl(returns(qualType(isAnyCharacter())))))))); const auto NegativeExpr = expr(ignoringParenImpCasts( unaryOperator(hasOperatorName("-"), hasUnaryOperand(integerLiteral(unless(equals(0))))))); @@ -76,6 +91,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 +107,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 +179,7 @@ if (Result.Nodes.getNodeAs("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 +210,13 @@ } diag(Loc, "constructing string from nullptr is undefined behaviour"); } + } else if (const auto *E = + Result.Nodes.getNodeAs("implicit-cast-both-args")) { + diag(Loc, "string constructor arguments might be incorrect;" + " calling as string(count, character) due to implicit casting of " + "both arguments;" + " use explicit casts if string(count, character) is the intended " + "constructor"); } } diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -139,6 +139,10 @@ ` check. Global options of the same name should be used instead. +- Updated :doc:`bugprone-string-constructor + ` check to flag more potentially + buggy code. + - Deprecated check-local options `HeaderFileExtensions` and `ImplementationFileExtensions` in :doc:`bugprone-suspicious-include ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/string-constructor.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/string-constructor.rst --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/string-constructor.rst +++ b/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: diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp @@ -5,12 +5,13 @@ class allocator {}; template class char_traits {}; -template , typename A = std::allocator > +template , typename A = std::allocator> 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 string; typedef basic_string wstring; @@ -18,7 +19,7 @@ template > 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 string_view; @@ -28,20 +29,58 @@ const char* kText = ""; const char kText2[] = ""; extern const char kText3[]; +char getChar(); +const char* getCharPtr(); 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; + char ch; + + 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 + // 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: string constructor arguments might be incorrect; + std::string s4(kText[1], 10); + // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments might be incorrect; + std::string s5(kText[1], i); + // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments might be incorrect; + std::string s6(*getCharPtr(), 10); + // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments might be incorrect; std::string q0("test", 0); // CHECK-MESSAGES: [[@LINE-1]]:15: warning: constructor creating an empty string @@ -91,12 +130,15 @@ } 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); std::string_view emptyv(); std::string_view sv1("test", 4);