niko created this revision. niko added reviewers: ioeric, hokein. Herald added subscribers: cfe-commits, mgorny.
Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43847 Files: clang-tidy/CMakeLists.txt clang-tidy/absl/AbslTidyModule.cpp clang-tidy/absl/CMakeLists.txt clang-tidy/absl/StringFindStartswithCheck.cpp clang-tidy/absl/StringFindStartswithCheck.h clang-tidy/plugin/CMakeLists.txt clang-tidy/tool/CMakeLists.txt clang-tidy/tool/ClangTidyMain.cpp docs/ReleaseNotes.rst docs/clang-tidy/checks/absl-string-find-startswith.rst docs/clang-tidy/checks/list.rst test/clang-tidy/absl-string-find-startswith.cpp
Index: test/clang-tidy/absl-string-find-startswith.cpp =================================================================== --- test/clang-tidy/absl-string-find-startswith.cpp +++ test/clang-tidy/absl-string-find-startswith.cpp @@ -0,0 +1,49 @@ +// RUN: %check_clang_tidy %s absl-string-find-startswith %t -- -- -I%S +// -std=c++11 + +namespace std { +template <typename T> class allocator {}; +template <typename T> class char_traits {}; +template <typename C, typename T = std::char_traits<C>, + typename A = std::allocator<C>> +struct basic_string { + basic_string(); + basic_string(const basic_string &); + basic_string(const C *, const A &a = A()); + ~basic_string(); + int find(basic_string<C> s, int pos = 0); + int find(const char *s, int pos = 0); +}; +typedef basic_string<char> string; +typedef basic_string<wchar_t> wstring; +} // namespace std + +std::string foo(std::string); +std::string bar(); + +void tests(std::string s) { + s.find("a") == 0; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use absl::StartsWith instead of + // find() == 0 [absl-string-find-startswith] CHECK_FIXES: + // {{^}}absl::StartsWith(s, "a");{{$}} + + s.find(s) == 0; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use absl::StartsWith instead of + // find() == 0 [absl-string-find-startswith] CHECK_FIXES: + // {{^}}absl::StartsWith(s, s);{{$}} + + s.find("aaa") != 0; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use !absl::StartsWith instead of + // find() != 0 [absl-string-find-startswith] CHECK_FIXES: + // {{^}}!absl::StartsWith(s, "aaa");{{$}} + + s.find(foo(foo(bar()))) != 0; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use !absl::StartsWith instead of + // find() != 0 [absl-string-find-startswith] CHECK_FIXES: + // {{^}}!absl::StartsWith(s, foo(foo(foo)));{{$}} + + // expressions that don't trigger the check are here. + s.find("a", 1) == 0; + s.find("a", 1) == 1; + s.find("a") == 1; +} Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -4,6 +4,7 @@ ================= .. toctree:: + absl-string-find-startswith android-cloexec-accept android-cloexec-accept4 android-cloexec-creat Index: docs/clang-tidy/checks/absl-string-find-startswith.rst =================================================================== --- docs/clang-tidy/checks/absl-string-find-startswith.rst +++ docs/clang-tidy/checks/absl-string-find-startswith.rst @@ -0,0 +1,18 @@ +.. title:: clang-tidy - absl-string-find-startswith + +absl-string-find-startswith +========================================== + +This check triggers on (in)equality comparisons between string.find() +and zero. Comparisons like this should be replaced with +absl::StartsWith(string, prefix). This is both a readability and a +performance issue. + +:: + + string s = "..."; + if (s.find("Hello World") == 0) { /* do something */ } + +should be replaced with +``if (absl::StartsWith(s, "Hello World")) { /* do something */ };`` + Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -57,6 +57,12 @@ Improvements to clang-tidy -------------------------- +- New `absl-string-find-startswith + <http://clang.llvm.org/extra/clang-tidy/checks/absl-string-find-startswith.html>`_ check + + Checks whether a string::find result is compared with 0, and suggests + replacing with absl::StartsWith. + - New `fuchsia-statically-constructed-objects <http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-statically-constructed-objects.html>`_ check Index: clang-tidy/tool/ClangTidyMain.cpp =================================================================== --- clang-tidy/tool/ClangTidyMain.cpp +++ clang-tidy/tool/ClangTidyMain.cpp @@ -462,6 +462,11 @@ static int LLVM_ATTRIBUTE_UNUSED CERTModuleAnchorDestination = CERTModuleAnchorSource; +// This anchor is used to force the linker to link the AbslModule. +extern volatile int AbslModuleAnchorSource; +static int LLVM_ATTRIBUTE_UNUSED AbslModuleAnchorDestination = + AbslModuleAnchorSource; + // This anchor is used to force the linker to link the BoostModule. extern volatile int BoostModuleAnchorSource; static int LLVM_ATTRIBUTE_UNUSED BoostModuleAnchorDestination = Index: clang-tidy/tool/CMakeLists.txt =================================================================== --- clang-tidy/tool/CMakeLists.txt +++ clang-tidy/tool/CMakeLists.txt @@ -18,6 +18,7 @@ clangBasic clangTidy clangTidyAndroidModule + clangTidyAbslModule clangTidyBoostModule clangTidyBugproneModule clangTidyCERTModule Index: clang-tidy/plugin/CMakeLists.txt =================================================================== --- clang-tidy/plugin/CMakeLists.txt +++ clang-tidy/plugin/CMakeLists.txt @@ -9,6 +9,7 @@ clangSema clangTidy clangTidyAndroidModule + clangTidyAbslModule clangTidyBoostModule clangTidyCERTModule clangTidyCppCoreGuidelinesModule Index: clang-tidy/absl/StringFindStartswithCheck.h =================================================================== --- clang-tidy/absl/StringFindStartswithCheck.h +++ clang-tidy/absl/StringFindStartswithCheck.h @@ -0,0 +1,37 @@ +//===--- StringFindStartswithCheck.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_ABSL_STRING_FIND_STARTSWITH_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSL_STRING_FIND_STARTSWITH_H_ + +#include "../ClangTidy.h" +#include "../utils/IncludeInserter.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +namespace clang { +namespace tidy { +namespace absl { + +// Find string.find(...) == 0 comparisons and suggest replacing with StartsWith. +class StringFindStartswithCheck : public ClangTidyCheck { +public: + using ClangTidyCheck::ClangTidyCheck; + void registerPPCallbacks(CompilerInstance &compiler) override; + void registerMatchers(ast_matchers::MatchFinder *finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &result) override; + +private: + std::unique_ptr<clang::tidy::utils::IncludeInserter> include_inserter_; +}; + +} // namespace absl +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSL_STRING_FIND_STARTSWITH_H_ Index: clang-tidy/absl/StringFindStartswithCheck.cpp =================================================================== --- clang-tidy/absl/StringFindStartswithCheck.cpp +++ clang-tidy/absl/StringFindStartswithCheck.cpp @@ -0,0 +1,99 @@ +#include "StringFindStartswithCheck.h" + +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Frontend/CompilerInstance.h" + +using namespace clang::ast_matchers; // NOLINT: Too many names. + +namespace clang { +namespace tidy { +namespace absl { + +void StringFindStartswithCheck::registerMatchers(MatchFinder *finder) { + auto zero = integerLiteral(equals(0)); + // TODO(nikoweh/reviewers): There are a few other options here, + // like adding std::string or std::basic_string (?) + auto stringClassMatcher = anyOf(cxxRecordDecl(hasName("string")), + cxxRecordDecl(hasName("__versa_string")), + cxxRecordDecl(hasName("basic_string"))); + + auto stringFind = cxxMemberCallExpr( + // .find()-call on a string... + callee(cxxMethodDecl(hasName("find"), ofClass(stringClassMatcher))), + // ... with some search expression ... + hasArgument(0, expr().bind("needle")), + // ... and either "0" as second argument or the default argument (also 0). + anyOf(hasArgument(1, zero), hasArgument(1, cxxDefaultArgExpr()))); + + finder->addMatcher( + // Match [=!]= with a zero on one side and a string.find on the other. + binaryOperator( + anyOf(hasOperatorName("=="), hasOperatorName("!=")), + hasEitherOperand(ignoringParenImpCasts(zero)), + hasEitherOperand(ignoringParenImpCasts(stringFind.bind("findexpr")))) + .bind("expr"), + this); +} + +void StringFindStartswithCheck::check(const MatchFinder::MatchResult &result) { + const auto &context = *result.Context; + const auto &source = context.getSourceManager(); + + // Extract matching (sub)expressions + const auto *expr = result.Nodes.getNodeAs<BinaryOperator>("expr"); + const auto *needle = result.Nodes.getNodeAs<Expr>("needle"); + const auto *haystack = result.Nodes.getNodeAs<CXXMemberCallExpr>("findexpr") + ->getImplicitObjectArgument(); + + // Get the source code blocks (as characters) for both the string object + // and the search expression + const StringRef needle_expr_code = Lexer::getSourceText( + CharSourceRange::getTokenRange(needle->getSourceRange()), source, + context.getLangOpts()); + const StringRef haystack_expr_code = Lexer::getSourceText( + CharSourceRange::getTokenRange(haystack->getSourceRange()), source, + context.getLangOpts()); + + // Create the StartsWith string, negating if comparison was "!=". + bool neg = expr->getOpcodeStr() == "!="; + StringRef startswith_str; + if (neg) { + startswith_str = "!absl::StartsWith"; + } else { + startswith_str = "absl::StartsWith"; + } + + // Create the warning message and a FixIt hint replacing the original expr. + auto diag_out = diag(expr->getLocStart(), + (StringRef("Use ") + startswith_str + + " instead of find() " + expr->getOpcodeStr() + " 0") + .str()) + << FixItHint::CreateReplacement(expr->getSourceRange(), + (startswith_str + "(" + + haystack_expr_code + ", " + + needle_expr_code + ")") + .str()); + + // Create a preprocessor #include FixIt hint (CreateIncludeInsertion checks + // whether this already exists). + auto include_hint = include_inserter_->CreateIncludeInsertion( + source.getFileID(expr->getLocStart()), "third_party/absl/strings/match.h", + false); + if (include_hint) { + diag_out << *include_hint; + } +} + +void StringFindStartswithCheck::registerPPCallbacks( + CompilerInstance &compiler) { + include_inserter_ = llvm::make_unique<clang::tidy::utils::IncludeInserter>( + compiler.getSourceManager(), compiler.getLangOpts(), + clang::tidy::utils::IncludeSorter::IS_Google); + compiler.getPreprocessor().addPPCallbacks( + include_inserter_->CreatePPCallbacks()); +} + +} // namespace absl +} // namespace tidy +} // namespace clang Index: clang-tidy/absl/CMakeLists.txt =================================================================== --- clang-tidy/absl/CMakeLists.txt +++ clang-tidy/absl/CMakeLists.txt @@ -0,0 +1,14 @@ +set(LLVM_LINK_COMPONENTS support) + +add_clang_library(clangTidyAbslModule + AbslTidyModule.cpp + StringFindStartswithCheck.cpp + + LINK_LIBS + clangAST + clangASTMatchers + clangBasic + clangLex + clangTidy + clangTidyUtils + ) Index: clang-tidy/absl/AbslTidyModule.cpp =================================================================== --- clang-tidy/absl/AbslTidyModule.cpp +++ clang-tidy/absl/AbslTidyModule.cpp @@ -0,0 +1,39 @@ +//===------- AbslTidyModule.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 "../ClangTidy.h" +#include "../ClangTidyModule.h" +#include "../ClangTidyModuleRegistry.h" +#include "StringFindStartswithCheck.h" +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace absl { + +class AbslModule : public ClangTidyModule { +public: + void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck<StringFindStartswithCheck>( + "absl-string-find-startswith"); + } +}; + +// Register the AbslModule using this statically initialized variable. +static ClangTidyModuleRegistry::Add<AbslModule> X("absl-module", + "Add absl checks."); + +} // namespace absl + +// This anchor is used to force the linker to link in the generated object file +// and thus register the AbslModule. +volatile int AbslModuleAnchorSource = 0; + +} // namespace tidy +} // namespace clang Index: clang-tidy/CMakeLists.txt =================================================================== --- clang-tidy/CMakeLists.txt +++ clang-tidy/CMakeLists.txt @@ -27,6 +27,7 @@ ) add_subdirectory(android) +add_subdirectory(absl) add_subdirectory(boost) add_subdirectory(bugprone) add_subdirectory(cert)
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits