diff options
8 files changed, 295 insertions, 0 deletions
diff --git a/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp b/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp index 178a7a610a0..e70184bfb3d 100644 --- a/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp @@ -14,6 +14,7 @@ #include "FasterStrsplitDelimiterCheck.h" #include "NoNamespaceCheck.h" #include "StringFindStartswithCheck.h" +#include "StrCatAppendCheck.h" namespace clang { namespace tidy { @@ -29,6 +30,8 @@ public: CheckFactories.registerCheck<NoNamespaceCheck>("abseil-no-namespace"); CheckFactories.registerCheck<StringFindStartswithCheck>( "abseil-string-find-startswith"); + CheckFactories.registerCheck<StrCatAppendCheck>( + "abseil-str-cat-append"); } }; diff --git a/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt b/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt index 601dceb12c2..97932013a6f 100644 --- a/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt @@ -6,6 +6,7 @@ add_clang_library(clangTidyAbseilModule FasterStrsplitDelimiterCheck.cpp NoNamespaceCheck.cpp StringFindStartswithCheck.cpp + StrCatAppendCheck.cpp LINK_LIBS clangAST diff --git a/clang-tools-extra/clang-tidy/abseil/StrCatAppendCheck.cpp b/clang-tools-extra/clang-tidy/abseil/StrCatAppendCheck.cpp new file mode 100644 index 00000000000..25b9d17e833 --- /dev/null +++ b/clang-tools-extra/clang-tidy/abseil/StrCatAppendCheck.cpp @@ -0,0 +1,102 @@ +//===--- StrCatAppendCheck.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 "StrCatAppendCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace abseil { + +namespace { +// Skips any combination of temporary materialization, temporary binding and +// implicit casting. +AST_MATCHER_P(Stmt, IgnoringTemporaries, ast_matchers::internal::Matcher<Stmt>, + InnerMatcher) { + const Stmt *E = &Node; + while (true) { + if (const auto *MTE = dyn_cast<MaterializeTemporaryExpr>(E)) + E = MTE->getTemporary(); + if (const auto *BTE = dyn_cast<CXXBindTemporaryExpr>(E)) + E = BTE->getSubExpr(); + if (const auto *ICE = dyn_cast<ImplicitCastExpr>(E)) + E = ICE->getSubExpr(); + else + break; + } + + return InnerMatcher.matches(*E, Finder, Builder); +} + +} // namespace + +// TODO: str += StrCat(...) +// str.append(StrCat(...)) + +void StrCatAppendCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus) + return; + const auto StrCat = functionDecl(hasName("::absl::StrCat")); + // The arguments of absl::StrCat are implicitly converted to AlphaNum. This + // matches to the arguments because of that behavior. + const auto AlphaNum = IgnoringTemporaries(cxxConstructExpr( + argumentCountIs(1), hasType(cxxRecordDecl(hasName("::absl::AlphaNum"))), + hasArgument(0, ignoringImpCasts(declRefExpr(to(equalsBoundNode("LHS")), + expr().bind("Arg0")))))); + + const auto HasAnotherReferenceToLhs = + callExpr(hasAnyArgument(expr(hasDescendant(declRefExpr( + to(equalsBoundNode("LHS")), unless(equalsBoundNode("Arg0"))))))); + + // Now look for calls to operator= with an object on the LHS and a call to + // StrCat on the RHS. The first argument of the StrCat call should be the same + // as the LHS. Ignore calls from template instantiations. + Finder->addMatcher( + cxxOperatorCallExpr( + unless(isInTemplateInstantiation()), hasOverloadedOperatorName("="), + hasArgument(0, declRefExpr(to(decl().bind("LHS")))), + hasArgument(1, IgnoringTemporaries( + callExpr(callee(StrCat), hasArgument(0, AlphaNum), + unless(HasAnotherReferenceToLhs)) + .bind("Call")))) + .bind("Op"), + this); +} + +void StrCatAppendCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Op = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("Op"); + const auto *Call = Result.Nodes.getNodeAs<CallExpr>("Call"); + assert(Op != nullptr && Call != nullptr && "Matcher does not work as expected"); + + // Handles the case 'x = absl::StrCat(x)', which has no effect. + if (Call->getNumArgs() == 1) { + diag(Op->getBeginLoc(), "call to 'absl::StrCat' has no effect"); + return; + } + + // Emit a warning and emit fixits to go from + // x = absl::StrCat(x, ...) + // to + // absl::StrAppend(&x, ...) + diag(Op->getBeginLoc(), + "call 'absl::StrAppend' instead of 'absl::StrCat' when appending to a " + "string to avoid a performance penalty") + << FixItHint::CreateReplacement( + CharSourceRange::getTokenRange(Op->getBeginLoc(), + Call->getCallee()->getEndLoc()), + "StrAppend") + << FixItHint::CreateInsertion(Call->getArg(0)->getBeginLoc(), "&"); +} + +} // namespace abseil +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/abseil/StrCatAppendCheck.h b/clang-tools-extra/clang-tidy/abseil/StrCatAppendCheck.h new file mode 100644 index 00000000000..eaa750daf32 --- /dev/null +++ b/clang-tools-extra/clang-tidy/abseil/StrCatAppendCheck.h @@ -0,0 +1,36 @@ +//===--- StrCatAppendCheck.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_ABSEIL_STRCATAPPENDCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_STRCATAPPENDCHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace abseil { + +/// Flags uses of absl::StrCat to append to a string. Suggests absl::StrAppend +/// should be used instead. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-str-cat-append.html +class StrCatAppendCheck : public ClangTidyCheck { +public: + StrCatAppendCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace abseil +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_STRCATAPPENDCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 6f39c924e83..d1d7f4a1f62 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -76,6 +76,12 @@ Improvements to clang-tidy Ensures code does not open ``namespace absl`` as that violates Abseil's compatibility guidelines. +- New :doc:`abseil-str-cat-append + <clang-tidy/checks/abseil-str-cat-append>` check. + + Flags uses of ``absl::StrCat()`` to append to a ``std::string``. Suggests + ``absl::StrAppend()`` should be used instead. + - New :doc:`readability-magic-numbers <clang-tidy/checks/readability-magic-numbers>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/abseil-str-cat-append.rst b/clang-tools-extra/docs/clang-tidy/checks/abseil-str-cat-append.rst new file mode 100644 index 00000000000..7ab1069b567 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/abseil-str-cat-append.rst @@ -0,0 +1,17 @@ +.. title:: clang-tidy - abseil-str-cat-append + +abseil-str-cat-append +===================== + +Flags uses of ``absl::StrCat()`` to append to a ``std::string``. Suggests +``absl::StrAppend()`` should be used instead. + +The extra calls cause unnecessary temporary strings to be constructed. Removing +them makes the code smaller and faster. + +.. code-block:: c++ + + a = absl::StrCat(a, b); // Use absl::StrAppend(&a, b) instead. + +Does not diagnose cases where ``abls::StrCat()`` is used as a template +argument for a functor. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index c52f030a6d1..bde9285fb6a 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -8,6 +8,7 @@ Clang-Tidy Checks abseil-faster-strsplit-delimiter abseil-no-namespace abseil-string-find-startswith + abseil-str-cat-append android-cloexec-accept android-cloexec-accept4 android-cloexec-creat diff --git a/clang-tools-extra/test/clang-tidy/abseil-str-cat-append.cpp b/clang-tools-extra/test/clang-tidy/abseil-str-cat-append.cpp new file mode 100644 index 00000000000..9a12733880a --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/abseil-str-cat-append.cpp @@ -0,0 +1,129 @@ +// RUN: %check_clang_tidy %s abseil-str-cat-append %t -- -- -I%S -std=c++11 + +typedef unsigned __INT16_TYPE__ char16; +typedef unsigned __INT32_TYPE__ char32; +typedef __SIZE_TYPE__ size; + +namespace std { +template <typename T> +class allocator {}; +template <typename T> +class char_traits {}; +template <typename C, typename T, typename A> +struct basic_string { + typedef basic_string<C, T, A> _Type; + basic_string(); + basic_string(const C* p, const A& a = A()); + + const C* c_str() const; + const C* data() const; + + _Type& append(const C* s); + _Type& append(const C* s, size n); + _Type& assign(const C* s); + _Type& assign(const C* s, size n); + + int compare(const _Type&) const; + int compare(const C* s) const; + int compare(size pos, size len, const _Type&) const; + int compare(size pos, size len, const C* s) const; + + size find(const _Type& str, size pos = 0) const; + size find(const C* s, size pos = 0) const; + size find(const C* s, size pos, size n) const; + + _Type& insert(size pos, const _Type& str); + _Type& insert(size pos, const C* s); + _Type& insert(size pos, const C* s, size n); + + _Type& operator+=(const _Type& str); + _Type& operator+=(const C* s); + _Type& operator=(const _Type& str); + _Type& operator=(const C* s); +}; + +typedef basic_string<char, std::char_traits<char>, std::allocator<char>> string; +typedef basic_string<wchar_t, std::char_traits<wchar_t>, + std::allocator<wchar_t>> + wstring; +typedef basic_string<char16, std::char_traits<char16>, std::allocator<char16>> + u16string; +typedef basic_string<char32, std::char_traits<char32>, std::allocator<char32>> + u32string; +} // namespace std + +std::string operator+(const std::string&, const std::string&); +std::string operator+(const std::string&, const char*); +std::string operator+(const char*, const std::string&); + +bool operator==(const std::string&, const std::string&); +bool operator==(const std::string&, const char*); +bool operator==(const char*, const std::string&); + +namespace llvm { +struct StringRef { + StringRef(const char* p); + StringRef(const std::string&); +}; +} // namespace llvm + +namespace absl { + +struct AlphaNum { + AlphaNum(int i); + AlphaNum(double f); + AlphaNum(const char* c_str); + AlphaNum(const std::string& str); + + private: + AlphaNum(const AlphaNum&); + AlphaNum& operator=(const AlphaNum&); +}; + +std::string StrCat(const AlphaNum& A); +std::string StrCat(const AlphaNum& A, const AlphaNum& B); + +template <typename A> +void Foo(A& a) { + a = StrCat(a); +} + +void Bar() { + std::string A, B; + Foo<std::string>(A); + + std::string C = StrCat(A); + A = StrCat(A); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: call to 'absl::StrCat' has no effect + A = StrCat(A, B); +// CHECK-MESSAGES: [[@LINE-1]]:3: warning: call 'absl::StrAppend' instead of 'absl::StrCat' when appending to a string to avoid a performance penalty +// CHECK-FIXES: {{^}} StrAppend(&A, B); + B = StrCat(A, B); + +#define M(X) X = StrCat(X, A) + M(B); +// CHECK-MESSAGES: [[@LINE-1]]:5: warning: call 'absl::StrAppend' instead of 'absl::StrCat' when appending to a string to avoid a performance penalty +// CHECK-FIXES: #define M(X) X = StrCat(X, A) +} + +void Regression_SelfAppend() { + std::string A; + A = StrCat(A, A); +} + +} // namespace absl + +void OutsideAbsl() { + std::string A, B; + A = absl::StrCat(A, B); +// CHECK-MESSAGES: [[@LINE-1]]:3: warning: call 'absl::StrAppend' instead of 'absl::StrCat' when appending to a string to avoid a performance penalty +// CHECK-FIXES: {{^}} StrAppend(&A, B); +} + +void OutisdeUsingAbsl() { + std::string A, B; + using absl::StrCat; + A = StrCat(A, B); +// CHECK-MESSAGES: [[@LINE-1]]:3: warning: call 'absl::StrAppend' instead of 'absl::StrCat' when appending to a string to avoid a performance penalty +// CHECK-FIXES: {{^}} StrAppend(&A, B); +} |
