[clang-tidy] abseil-str-cat-append

[hugoeg]

Summary

Flags uses of absl::StrCat to append to a string. Suggests absl::StrAppend should be used instead.

The check was developed internally and has been running at google, this is just
a move to open source the check. It was originally written by @bkramer and has been modified for external use.

[Eugene.Zelenko]

clang-tidy/abseil/StrCatAppendCheck.cpp

28

while (true) will be better

51

Please run Clang-format.

docs/ReleaseNotes.rst

60

Please add new check entry in alphabetical order.

63

Please enclose absl::StrCat in `` and add (). same for absl::StrAppend. What does string? std::string?

docs/clang-tidy/checks/abseil-str-cat-append.rst

7

Please synchronize with Release Notes.

[hokein]

Please make sure the code follows the LLVM code style, e.g. the variable name format "CamelName".

[JonasToth]

clang-tidy/abseil/StrCatAppendCheck.cpp

29

You can ellide the braces for the single stmt ifs

53

The naming is ambigous. str_cat can easily be mixed with strcat. Is this constant even necessary or could you use the literal in the functionDecl matcher?

86

Please make this comment a full sentence

88

I think it would be better to include absl::StrCat in the diagnostic, as it is more clear.
Same opinion on the other diag.

test/clang-tidy/abseil-str-cat-append.cpp

92

What happens if StrCat is used e.g. in an std::accumulate call as the binary operator? (https://en.cppreference.com/w/cpp/algorithm/accumulate the Example includes such a case)
Is this diagnosed, too?

The general case would be if StrCat is used as a template argument for a functor.

115

Could you please add a testcase that is outside of the absl namespace.

void f() {
  std::string s;
  s = absl::StrCat(s,s);
}

and

void g() {
  std::string s;
  using absl::StrCat;
  s = StrCat(s, s);
}

with better naming.

[hugoeg]

applied corrections from comments

[hugoeg]

test/clang-tidy/abseil-str-cat-append.cpp

92

it doesn't diagnose those cases. and diagnosing it doesn't seem to be worthwhile

[hugoeg]

minor fixes

[JonasToth]

test/clang-tidy/abseil-str-cat-append.cpp

92

You could add it as a known limitation to the docs / or as TODO in the code.

[Eugene.Zelenko]

docs/ReleaseNotes.rst

70

Please enclose std::string into ``

72

Details belongs to documentation.

[hugoeg]

Let me know if there's anything else I can fix to move the process along.

[Eugene.Zelenko]

docs/clang-tidy/checks/abseil-str-cat-append.rst

12

Please add namespace.

[hugoeg]

minor fixes, style improvements

[JonasToth]

clang-tidy/abseil/StrCatAppendCheck.cpp

22

I think you can elide the empty line around the matcher.

50

Not sure if this comment adds value and might even be a little misleading. StrCat itself does not look for calls, it is a helper to do so.
Maybe you can move the comment to the actual matching part with everything composed and explain more what is specifically looked for.

52

Please make this comment a sentence. Match that the arguments ... would be ok i think.

58

Please rename this variable to follow the convention

80

please add an assert(Op != nullptr && Call != nullptr && "Matcher does not work as expected"), even though it seems trivial its better then nullptr deref.
And the logic might change in the future, so better be save

81

I think // Handles the case 'x = absl::StrCat(x)', which does nothing. would be better

83

please use 'absl::StrCat' in the diagnostics (same below) to signify that it is code.

[aaron.ballman]

clang-tidy/abseil/StrCatAppendCheck.cpp

83

"does nothing" is not quite correct either; it does something, just not what the user expects. How about call to 'absl::StrCat' has no effect?

92–93

This wins the award for "kindest diagnostic message". :-D How about: call 'absl::StrAppend' instead of 'absl::StrCat' when appending to a string?

That said, the diagnostic doesn't say *why* the code is wrong in the first place. The documentation doesn't really go into it either. Is it a correctness issue, a performance issue, something else?

[hugoeg]

applied corrections from comments

[hugoeg]

ran svn update

[aaron.ballman]

clang-tidy/abseil/StrCatAppendCheck.cpp

92–93

So this is a performance concern, not a correctness concern? In that case, how about: call 'absl::StrAppend' instead of 'absl::StrCat' to avoid a performance penalty when appending to a string?

[aaron.ballman]

LGTM!

[hugoeg]

@aaron.ballman when you get the chance could you take another look at this and commit if it is ready? My internship ends rather soon and this is a tad time sensitive and I don't have commit access. Thank you for your time!

[aaron.ballman]

In D51061#1215917, @hugoeg wrote:

@aaron.ballman when you get the chance could you take another look at this and commit if it is ready? My internship ends rather soon and this is a tad time sensitive and I don't have commit access. Thank you for your time!

I've commit in r340915. Thank you for the patch!

[hokein]

Thanks @aaron.ballman. I'm closing it now as it is committed in rL340915.

clang-tidy/abseil/AbseilTidyModule.cpp

 #include "DurationDivisionCheck.h"
 #include "FasterStrsplitDelimiterCheck.h"
 #include "StringFindStartswithCheck.h"
+#include "StrCatAppendCheck.h"
 
 namespace clang {
 namespace tidy {

         "abseil-faster-strsplit-delimiter");
     CheckFactories.registerCheck<StringFindStartswithCheck>(
         "abseil-string-find-startswith");
+    CheckFactories.registerCheck<StrCatAppendCheck>(
+        "abseil-str-cat-append");
   }
 };
 

clang-tidy/abseil/CMakeLists.txt

   DurationDivisionCheck.cpp
   FasterStrsplitDelimiterCheck.cpp
   StringFindStartswithCheck.cpp
+  StrCatAppendCheck.cpp
 
   LINK_LIBS
   clangAST

clang-tidy/abseil/StrCatAppendCheck.h

+//===--- 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-tidy/abseil/StrCatAppendCheck.cpp

+//===--- 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

docs/ReleaseNotes.rst

   Finds instances of ``absl::StrSplit()`` or ``absl::MaxSplits()`` where the
   delimiter is a single character string literal and replaces with a character.
 
+- 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.
 

docs/clang-tidy/checks/abseil-str-cat-append.rst

+.. 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.

docs/clang-tidy/checks/list.rst

    abseil-duration-division
    abseil-faster-strsplit-delimiter
    abseil-string-find-startswith
+   abseil-str-cat-append
    android-cloexec-accept
    android-cloexec-accept4
    android-cloexec-creat

test/clang-tidy/abseil-str-cat-append.cpp

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