Introduce the abseil-str-cat-append check.

This flags uses of absl::StrCat when absl::StrAppend should be used instead. Patch by Hugo Gonzalez and Benjamin Kramer.

llvm-svn: 340915

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");
   }
 };
 

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

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

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

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.
 

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.

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

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);
+}