Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -25,6 +25,7 @@ SizeofContainerCheck.cpp SizeofExpressionCheck.cpp StaticAssertCheck.cpp + StringConstructorCheck.cpp StringIntegerAssignmentCheck.cpp StringLiteralWithEmbeddedNulCheck.cpp SuspiciousMissingCommaCheck.cpp Index: clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tidy/misc/MiscTidyModule.cpp +++ clang-tidy/misc/MiscTidyModule.cpp @@ -33,6 +33,7 @@ #include "SizeofContainerCheck.h" #include "SizeofExpressionCheck.h" #include "StaticAssertCheck.h" +#include "StringConstructorCheck.h" #include "StringIntegerAssignmentCheck.h" #include "StringLiteralWithEmbeddedNulCheck.h" #include "SuspiciousMissingCommaCheck.h" @@ -97,6 +98,8 @@ "misc-sizeof-expression"); CheckFactories.registerCheck( "misc-static-assert"); + CheckFactories.registerCheck( + "misc-string-constructor"); CheckFactories.registerCheck( "misc-string-integer-assignment"); CheckFactories.registerCheck( Index: clang-tidy/misc/StringConstructorCheck.h =================================================================== --- /dev/null +++ clang-tidy/misc/StringConstructorCheck.h @@ -0,0 +1,39 @@ +//===--- StringConstructorCheck.h - clang-tidy-------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STRING_CONSTRUCTOR_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STRING_CONSTRUCTOR_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace misc { + +/// Finds suspicious string constructor and check their parameters. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-string-constructor.html +class StringConstructorCheck : public ClangTidyCheck { +public: + StringConstructorCheck(StringRef Name, ClangTidyContext *Context); + void storeOptions(ClangTidyOptions::OptionMap &Opts); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + const bool WarnOnLargeLength; + const unsigned int LargeLengthThreshold; +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STRING_CONSTRUCTOR_H Index: clang-tidy/misc/StringConstructorCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/misc/StringConstructorCheck.cpp @@ -0,0 +1,126 @@ +//===--- StringConstructorCheck.cpp - clang-tidy---------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "StringConstructorCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace misc { + +AST_MATCHER_P(IntegerLiteral, isBiggerThan, unsigned, N) { + return Node.getValue().getZExtValue() > N; +} + +StringConstructorCheck::StringConstructorCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + WarnOnLargeLength(Options.get("WarnOnLargeLength", 1) != 0), + LargeLengthThreshold(Options.get("LargeLengthThreshold", 0x800000)) {} + +void StringConstructorCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "WarnOnLargeLength", WarnOnLargeLength); + Options.store(Opts, "LargeLengthThreshold", LargeLengthThreshold); +} + +void StringConstructorCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus) + return; + + const auto ZeroExpr = expr(ignoringParenImpCasts(integerLiteral(equals(0)))); + const auto CharExpr = expr(ignoringParenImpCasts(characterLiteral())); + const auto NegativeExpr = expr(ignoringParenImpCasts( + unaryOperator(hasOperatorName("-"), + hasUnaryOperand(integerLiteral(unless(equals(0))))))); + const auto LargeLengthExpr = expr(ignoringParenImpCasts( + integerLiteral(isBiggerThan(LargeLengthThreshold)))); + const auto CharPtrType = type(anyOf(pointerType(), arrayType())); + + // Match a string-literal; even through a declaration with initializer. + const auto BoundStringLiteral = stringLiteral().bind("str"); + const auto ConstStrLiteralDecl = varDecl( + isDefinition(), hasType(constantArrayType()), hasType(isConstQualified()), + hasInitializer(ignoringParenImpCasts(BoundStringLiteral))); + const auto ConstPtrStrLiteralDecl = varDecl( + isDefinition(), + hasType(pointerType(pointee(isAnyCharacter(), isConstQualified()))), + hasInitializer(ignoringParenImpCasts(BoundStringLiteral))); + auto ConstStrLiteral = expr(ignoringParenImpCasts(anyOf( + BoundStringLiteral, declRefExpr(hasDeclaration(anyOf( + ConstPtrStrLiteralDecl, ConstStrLiteralDecl)))))); + + // Check the fill constructor. Fills the string with n consecutive copies of + // character c. [i.e string(size_t n, char c);]. + Finder->addMatcher( + cxxConstructExpr( + 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")))) + .bind("constructor"), + this); + + // Check the literal string constructor with char pointer and length + // parameters. [i.e. string (const char* s, size_t n);] + Finder->addMatcher( + cxxConstructExpr( + hasDeclaration(cxxMethodDecl(hasName("basic_string"))), + hasArgument(0, hasType(CharPtrType)), + hasArgument(1, hasType(isInteger())), + anyOf( + // Detect the expression: string("...", 0); + hasArgument(1, ZeroExpr.bind("empty-string")), + // Detect the expression: string("...", -4); + hasArgument(1, NegativeExpr.bind("negative-length")), + // Detect the expression: string("lit", 0x1234567); + hasArgument(1, LargeLengthExpr.bind("large-length")), + // Detect the expression: string("lit", 5) + allOf(hasArgument(0, ConstStrLiteral.bind("literal-with-length")), + hasArgument(1, ignoringParenImpCasts( + integerLiteral().bind("int")))))) + .bind("constructor"), + this); +} + +void StringConstructorCheck::check(const MatchFinder::MatchResult &Result) { + const auto *E = Result.Nodes.getNodeAs("constructor"); + SourceLocation Loc = E->getLocStart(); + + if (Result.Nodes.getNodeAs("swapped-parameter")) { + diag(Loc, "constructor parameters are probably swapped"); + } else if (Result.Nodes.getNodeAs("empty-string")) { + diag(Loc, "constructor creating an empty string"); + } else if (Result.Nodes.getNodeAs("negative-length")) { + diag(Loc, "negative value used as length parameter"); + } else if (Result.Nodes.getNodeAs("large-length")) { + if (WarnOnLargeLength) + diag(Loc, "suspicious large length parameter"); + } else if (Result.Nodes.getNodeAs("literal-with-length")) { + const auto *S = Result.Nodes.getNodeAs("str"); + const auto *L = Result.Nodes.getNodeAs("int"); + if (L->getValue().ugt(S->getLength())) { + diag(Loc, "length is bigger then string literal size"); + } + } +} + +} // namespace misc +} // namespace tidy +} // namespace clang Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -69,6 +69,7 @@ misc-sizeof-container misc-sizeof-expression misc-static-assert + misc-string-constructor misc-string-integer-assignment misc-string-literal-with-embedded-nul misc-suspicious-missing-comma Index: docs/clang-tidy/checks/misc-string-constructor.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/misc-string-constructor.rst @@ -0,0 +1,36 @@ +.. title:: clang-tidy - misc-string-constructor + +misc-string-constructor +======================= + +Finds string constructors that are suspicious and probably errors. + + +A common mistake is to swap parameters to the 'fill' string-constructor. + +Examples: + +.. code:: c++ + + std::string('x', 50) str; // should be std::string(50, 'x') + + +Calling the string-literal constructor with a length bigger than the literal is +suspicious and adds extra random characters to the string. + +Examples: + +.. code:: c++ + + std::string("test", 200); // Will include random characters after "test". + + +Creating an empty string from constructors with parameters is considered +suspicious. The programmer should use the empty constructor instead. + +Examples: + +.. code:: c++ + + std::string("test", 0); // Creation of an empty string. + Index: test/clang-tidy/misc-string-constructor.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-string-constructor.cpp @@ -0,0 +1,54 @@ +// RUN: %check_clang_tidy %s misc-string-constructor %t + +namespace std { +template +class allocator {}; +template +class char_traits {}; +template , typename A = std::allocator > +struct basic_string { + basic_string(); + basic_string(const C*, unsigned int size); + basic_string(unsigned int size, C c); +}; +typedef basic_string string; +typedef basic_string wstring; +} + +const char* kText = ""; +const char kText2[] = ""; +extern const char kText3[]; + +void Test() { + std::string str('x', 4); + // CHECK-MESSAGES: [[@LINE-1]]:15: warning: constructor parameters are probably swapped [misc-string-constructor] + std::wstring wstr(L'x', 4); + // CHECK-MESSAGES: [[@LINE-1]]:16: warning: constructor parameters are probably swapped + 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 q0("test", 0); + // CHECK-MESSAGES: [[@LINE-1]]:15: warning: constructor creating an empty string + std::string q1(kText, -4); + // CHECK-MESSAGES: [[@LINE-1]]:15: warning: negative value used as length parameter + std::string q2("test", 200); + // CHECK-MESSAGES: [[@LINE-1]]:15: warning: length is bigger then string literal size + std::string q3(kText, 200); + // CHECK-MESSAGES: [[@LINE-1]]:15: warning: length is bigger then string literal size + std::string q4(kText2, 200); + // CHECK-MESSAGES: [[@LINE-1]]:15: warning: length is bigger then string literal size + std::string q5(kText3, 0x1000000); + // CHECK-MESSAGES: [[@LINE-1]]:15: warning: suspicious large length parameter +} + +void Valid() { + std::string empty(); + std::string str(4, 'x'); + std::wstring wstr(4, L'x'); + std::string s1("test", 4); + std::string s2("test", 3); +}