[clang-tidy] abseil-redundant-strcat-calls-check

[hugoeg]

Summary

Flags redundant calls to `absl::StrCat when the result is being passed to another call of absl::StrCat/absl::StrAppend`. Also suggests a fix to collapse the calls.
Example:
StrCat(1, StrCat(2, 3)) ==> StrCat(1, 2, 3)

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 @sbenza and has been modified for external use.

[Eugene.Zelenko]

Is it possible to implement fix-it?

clang-tidy/abseil/RedundantStrcatCallsCheck.cpp

97

auto could be used because of iterator on container.

98

continue should be on own line. Please run Clang-format.

130

Return type is not obvious, so auto should not be used.

docs/ReleaseNotes.rst

76

Please remove This check

'' are used instead of ``.

docs/clang-tidy/checks/abseil-redundant-strcat-calls.rst

7

Please synchronize with Release Notes.

11

Please remove leading space.

14

Please add examples.

[hugoeg]

If the fix-it can be implemented, I certainly do not know how. As mentioned above, I am not the original author for this check.

[hugoeg]

corrections applied

[Eugene.Zelenko]

docs/clang-tidy/checks/abseil-redundant-strcat-calls.rst

17

Please add namespaces and use empty line instead of ==>

[Eugene.Zelenko]

docs/clang-tidy/checks/abseil-redundant-strcat-calls.rst

17

std::string. Please insert empty line.

18

std::string. Same indentation as in previous line. Same for second example.

[hugoeg]

minor fixes and style improvement

[aaron.ballman]

clang-tidy/abseil/RedundantStrcatCallsCheck.cpp

29

The formatting here looks off -- you should run the patch through clang-format, if you haven't already.

38–39

Can this be replaced by anyOf(CallToStrcat, CallToStrappend)?

51

s/hints/Hints

97

const auto * please (so it's obvious the type is a pointer).

100

s/sub/Sub

114–120

Elide braces

137

This text doesn't really help the user to understand what they've done wrong with their code. Perhaps multiple calls to 'StrCat' can be flattened into a single call or something along those lines?

[hugoeg]

applied corrections form comments

[hugoeg]

clang-tidy/abseil/RedundantStrcatCallsCheck.cpp

38–39

I just tried it, it doesn't work, so we should probably keep it as is.

[aaron.ballman]

clang-tidy/abseil/RedundantStrcatCallsCheck.cpp

29

This comment is marked done but the formatting in the patch still doesn't match the LLVM coding style. For instance, pointers bind to the right, not to the left in our style.

136

One more change, based on comments in other reviews, this should probably be 'absl::StrCat' instead of just 'StrCat'.

[hugoeg]

minor updates

[aaron.ballman]

Aside from the formatting and one small documentation nit, LGTM!

docs/clang-tidy/checks/abseil-redundant-strcat-calls.rst

8

to another -> to another call to

[hugoeg]

added corrections

[Eugene.Zelenko]

Example in documentation was not fixed.

docs/clang-tidy/checks/abseil-redundant-strcat-calls.rst

17

Will be good idea to add before/after comments. Indentation is not fixed and empty line was not inserted. Same or second example.

[Eugene.Zelenko]

Indentation of example in documentation is still fixed.

[hugoeg]

In D51132#1212903, @Eugene.Zelenko wrote:

Indentation of example in documentation is still fixed.

yea sorry about that, I caught that after i updated the diff and corrected and re updated

[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. Thank you for your time!

[aaron.ballman]

In D51132#1215916, @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. Thank you for your time!

When I try to apply your patch on top of ToT, I get merge conflicts. Can you resolve those and upload a new patch?

[hugoeg]

merge conflicts resolved @aaron.ballman

[aaron.ballman]

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

clang-tidy/abseil/AbseilTidyModule.cpp

 #include "DurationDivisionCheck.h"
 #include "FasterStrsplitDelimiterCheck.h"
 #include "NoNamespaceCheck.h"
+#include "RedundantStrcatCallsCheck.h"
 #include "StringFindStartswithCheck.h"
 
 namespace clang {

     CheckFactories.registerCheck<FasterStrsplitDelimiterCheck>(
         "abseil-faster-strsplit-delimiter");
     CheckFactories.registerCheck<NoNamespaceCheck>("abseil-no-namespace");
+    CheckFactories.registerCheck<RedundantStrcatCallsCheck>(
+        "abseil-redundant-strcat-calls");
     CheckFactories.registerCheck<StringFindStartswithCheck>(
         "abseil-string-find-startswith");
   }

clang-tidy/abseil/CMakeLists.txt

   DurationDivisionCheck.cpp
   FasterStrsplitDelimiterCheck.cpp
   NoNamespaceCheck.cpp
+  RedundantStrcatCallsCheck.cpp
   StringFindStartswithCheck.cpp
 
   LINK_LIBS

clang-tidy/abseil/RedundantStrcatCallsCheck.h

+//===--- RedundantStrcatCallsCheck.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_REDUNDANTSTRCATCALLSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_REDUNDANTSTRCATCALLSCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Flags redundant calls to absl::StrCat when the result is being passed to 
+/// another call of absl::StrCat/absl::StrAppend. Also suggests a fix to 
+/// collapse the calls.
+/// Example:
+///   StrCat(1, StrCat(2, 3))  ==>  StrCat(1, 2, 3)
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-redundant-strcat-calls.html
+class RedundantStrcatCallsCheck : public ClangTidyCheck {
+public:
+  RedundantStrcatCallsCheck(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_REDUNDANTSTRCATCALLSCHECK_H

clang-tidy/abseil/RedundantStrcatCallsCheck.cpp

+//===--- RedundantStrcatCallsCheck.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 "RedundantStrcatCallsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+// TODO: Features to add to the check:
+//  - Make it work if num_args > 26.
+//  - Remove empty literal string arguments.
+//  - Collapse consecutive literal string arguments into one (remove the ,).
+//  - Replace StrCat(a + b)  ->  StrCat(a, b)  if a or b are strings.
+//  - Make it work in macros if the outer and inner StrCats are both in the
+//    argument.
+
+void RedundantStrcatCallsCheck::registerMatchers(MatchFinder* Finder) {
+  if (!getLangOpts().CPlusPlus) 
+    return;
+  const auto CallToStrcat =
+      callExpr(callee(functionDecl(hasName("::absl::StrCat"))));
+  const auto CallToStrappend =
+      callExpr(callee(functionDecl(hasName("::absl::StrAppend"))));
+  // Do not match StrCat() calls that are descendants of other StrCat calls.
+  // Those are handled on the ancestor call.
+  const auto CallToEither = callExpr(
+      callee(functionDecl(hasAnyName("::absl::StrCat", "::absl::StrAppend"))));
+  Finder->addMatcher(
+      callExpr(CallToStrcat, unless(hasAncestor(CallToEither))).bind("StrCat"),
+      this);
+  Finder->addMatcher(CallToStrappend.bind("StrAppend"), this);
+}
+
+namespace {
+
+struct StrCatCheckResult {
+  int NumCalls = 0;
+  std::vector<FixItHint> Hints;
+};
+
+void RemoveCallLeaveArgs(const CallExpr* Call, StrCatCheckResult* CheckResult) {
+  // Remove 'Foo('
+  CheckResult->Hints.push_back(
+      FixItHint::CreateRemoval(CharSourceRange::getCharRange(
+          Call->getBeginLoc(), Call->getArg(0)->getBeginLoc())));
+  // Remove the ')'
+  CheckResult->Hints.push_back(
+      FixItHint::CreateRemoval(CharSourceRange::getCharRange(
+          Call->getRParenLoc(), Call->getEndLoc().getLocWithOffset(1))));
+}
+
+const clang::CallExpr* ProcessArgument(const Expr* Arg,
+                                       const MatchFinder::MatchResult& Result,
+                                       StrCatCheckResult* CheckResult) {
+  const auto IsAlphanum = hasDeclaration(cxxMethodDecl(hasName("AlphaNum")));
+  static const auto* const Strcat = new auto(hasName("::absl::StrCat"));
+  const auto IsStrcat = cxxBindTemporaryExpr(
+      has(callExpr(callee(functionDecl(*Strcat))).bind("StrCat")));
+  if (const auto* SubStrcatCall = selectFirst<const CallExpr>(
+          "StrCat",
+          match(stmt(anyOf(
+                    cxxConstructExpr(IsAlphanum, hasArgument(0, IsStrcat)),
+                    IsStrcat)),
+                *Arg->IgnoreParenImpCasts(), *Result.Context))) {
+    RemoveCallLeaveArgs(SubStrcatCall, CheckResult);
+    return SubStrcatCall;
+  }
+  return nullptr;
+}
+
+StrCatCheckResult ProcessCall(const CallExpr* RootCall, bool IsAppend,
+                              const MatchFinder::MatchResult& Result) {
+  StrCatCheckResult CheckResult;
+  std::deque<const CallExpr*> CallsToProcess = {RootCall};
+
+  while (!CallsToProcess.empty()) {
+    ++CheckResult.NumCalls;
+
+    const CallExpr* CallExpr = CallsToProcess.front();
+    CallsToProcess.pop_front();
+
+    int StartArg = CallExpr == RootCall && IsAppend;
+    for (const auto *Arg : CallExpr->arguments()) {
+      if (StartArg-- > 0) 
+        continue;
+      if (const clang::CallExpr* Sub =
+              ProcessArgument(Arg, Result, &CheckResult)) {
+        CallsToProcess.push_back(Sub);
+      }
+    }
+  }
+  return CheckResult;
+}
+}  // namespace
+
+void RedundantStrcatCallsCheck::check(const MatchFinder::MatchResult& Result) {
+  bool IsAppend;
+
+  const CallExpr* RootCall;
+  if ((RootCall = Result.Nodes.getNodeAs<CallExpr>("StrCat"))) 
+    IsAppend = false;
+  else if ((RootCall = Result.Nodes.getNodeAs<CallExpr>("StrAppend"))) 
+    IsAppend = true;
+  else 
+    return;
+
+  if (RootCall->getBeginLoc().isMacroID()) {
+    // Ignore calls within macros.
+    // In many cases the outer StrCat part of the macro and the inner StrCat is
+    // a macro argument. Removing the inner StrCat() converts one macro
+    // argument into many.
+    return;
+  }
+
+  const StrCatCheckResult CheckResult =
+      ProcessCall(RootCall, IsAppend, Result);
+  if (CheckResult.NumCalls == 1) {
+    // Just one call, so nothing to fix.
+    return;
+  }
+
+  diag(RootCall->getBeginLoc(), 
+       "multiple calls to 'absl::StrCat' can be flattened into a single call")
+      << CheckResult.Hints;
+}
+
+}  // namespace abseil
+}  // namespace tidy
+}  // namespace clang

docs/ReleaseNotes.rst

   delimiter is a single character string literal and replaces with a character.
 
 - New :doc:`abseil-no-namespace
-  <clang-tidy/checks/abseil-no-namespace>` check.
+  <clang-tidy/checks/abseil-no-namespace>` check. 
 
   Ensures code does not open ``namespace absl`` as that violates Abseil's
   compatibility guidelines.
 
+- New :doc:`abseil-redundant-strcat-calls
+  <clang-tidy/checks/abseil-redundant-strcat-calls>` check.
+
+  Suggests removal of unnecessary calls to ``absl::StrCat`` when the result is 
+  being passed to another ``absl::StrCat`` or ``absl::StrAppend``.
+
 - New :doc:`readability-magic-numbers
   <clang-tidy/checks/readability-magic-numbers>` check.
 

docs/clang-tidy/checks/abseil-redundant-strcat-calls.rst

+.. title:: clang-tidy - abseil-redundant-strcat-calls
+
+abseil-redundant-strcat-calls
+=============================
+
+Suggests removal of unnecessary calls to ``absl::StrCat`` when the result is 
+being passed to another call to ``absl::StrCat`` or ``absl::StrAppend``.
+
+The extra calls cause unnecessary temporary strings to be constructed. Removing
+them makes the code smaller and faster.
+
+Examples:
+
+.. code-block:: c++
+
+  std::string s = absl::StrCat("A", absl::StrCat("B", absl::StrCat("C", "D")));
+  //before
+
+  std::string s = absl::StrCat("A", "B", "C", "D");
+  //after
+
+  absl::StrAppend(&s, absl::StrCat("E", "F", "G"));
+  //before
+  
+  absl::StrAppend(&s, "E", "F", "G");
+  //after

docs/clang-tidy/checks/list.rst

    abseil-duration-division
    abseil-faster-strsplit-delimiter
    abseil-no-namespace
+   abseil-redundant-strcat-calls
    abseil-string-find-startswith
    android-cloexec-accept
    android-cloexec-accept4

test/clang-tidy/abseil-redundant-strcat-calls.cpp

+// RUN: %check_clang_tidy %s abseil-redundant-strcat-calls %t
+
+int strlen(const char *);
+
+// Here we mimic the hierarchy of ::string.
+// We need to do so because we are matching on the fully qualified name of the
+// methods.
+struct __sso_string_base {};
+namespace __gnu_cxx {
+template <typename A, typename B, typename C, typename D = __sso_string_base>
+class __versa_string {
+ public:
+  const char *c_str() const;
+  const char *data() const;
+  int size() const;
+  int capacity() const;
+  int length() const;
+  bool empty() const;
+  char &operator[](int);
+  void clear();
+  void resize(int);
+  int compare(const __versa_string &) const;
+};
+}  // namespace __gnu_cxx
+
+namespace std {
+template <typename T>
+class char_traits {};
+template <typename T>
+class allocator {};
+}  // namespace std
+
+template <typename A, typename B = std::char_traits<A>,
+          typename C = std::allocator<A>>
+class basic_string : public __gnu_cxx::__versa_string<A, B, C> {
+ public:
+  basic_string();
+  basic_string(const basic_string &);
+  basic_string(const char *, C = C());
+  basic_string(const char *, int, C = C());
+  basic_string(const basic_string &, int, int, C = C());
+  ~basic_string();
+
+  basic_string &operator+=(const basic_string &);
+};
+
+template <typename A, typename B, typename C>
+basic_string<A, B, C> operator+(const basic_string<A, B, C> &,
+                                const basic_string<A, B, C> &);
+template <typename A, typename B, typename C>
+basic_string<A, B, C> operator+(const basic_string<A, B, C> &, const char *);
+
+typedef basic_string<char> string;
+
+bool operator==(const string &, const string &);
+bool operator==(const string &, const char *);
+bool operator==(const char *, const string &);
+
+bool operator!=(const string &, const string &);
+bool operator<(const string &, const string &);
+bool operator>(const string &, const string &);
+bool operator<=(const string &, const string &);
+bool operator>=(const string &, const string &);
+
+namespace std {
+template <typename _CharT, typename _Traits = char_traits<_CharT>,
+          typename _Alloc = allocator<_CharT>>
+class basic_string;
+
+template <typename _CharT, typename _Traits, typename _Alloc>
+class basic_string {
+ public:
+  basic_string();
+  basic_string(const basic_string &);
+  basic_string(const char *, const _Alloc & = _Alloc());
+  basic_string(const char *, int, const _Alloc & = _Alloc());
+  basic_string(const basic_string &, int, int, const _Alloc & = _Alloc());
+  ~basic_string();
+
+  basic_string &operator+=(const basic_string &);
+
+  unsigned size() const;
+  unsigned length() const;
+  bool empty() const;
+};
+
+typedef basic_string<char> string;
+}  // namespace std
+
+namespace absl {
+
+class string_view {
+ public:
+  typedef std::char_traits<char> traits_type;
+
+  string_view();
+  string_view(const char *);
+  string_view(const string &);
+  string_view(const char *, int);
+  string_view(string_view, int);
+
+  template <typename A>
+  explicit operator ::basic_string<char, traits_type, A>() const;
+
+  const char *data() const;
+  int size() const;
+  int length() const;
+};
+
+bool operator==(string_view A, string_view B);
+
+struct AlphaNum {
+  AlphaNum(int i);
+  AlphaNum(double f);
+  AlphaNum(const char *c_str);
+  AlphaNum(const string &str);
+  AlphaNum(const string_view &pc);
+
+ private:
+  AlphaNum(const AlphaNum &);
+  AlphaNum &operator=(const AlphaNum &);
+};
+
+string StrCat(const AlphaNum &A);
+string StrCat(const AlphaNum &A, const AlphaNum &B);
+string StrCat(const AlphaNum &A, const AlphaNum &B, const AlphaNum &C);
+string StrCat(const AlphaNum &A, const AlphaNum &B, const AlphaNum &C,
+              const AlphaNum &D);
+
+// Support 5 or more arguments
+template <typename... AV>
+string StrCat(const AlphaNum &A, const AlphaNum &B, const AlphaNum &C,
+              const AlphaNum &D, const AlphaNum &E, const AV &... args);
+
+void StrAppend(string *Dest, const AlphaNum &A);
+void StrAppend(string *Dest, const AlphaNum &A, const AlphaNum &B);
+void StrAppend(string *Dest, const AlphaNum &A, const AlphaNum &B,
+               const AlphaNum &C);
+void StrAppend(string *Dest, const AlphaNum &A, const AlphaNum &B,
+               const AlphaNum &C, const AlphaNum &D);
+
+// Support 5 or more arguments
+template <typename... AV>
+void StrAppend(string *Dest, const AlphaNum &A, const AlphaNum &B,
+               const AlphaNum &C, const AlphaNum &D, const AlphaNum &E,
+               const AV &... args);
+
+}  // namespace absl
+
+using absl::AlphaNum;
+using absl::StrAppend;
+using absl::StrCat;
+
+void Positives() {
+  string S = StrCat(1, StrCat("A", StrCat(1.1)));
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: multiple calls to 'absl::StrCat' can be flattened into a single call
+
+  S = StrCat(StrCat(StrCat(StrCat(StrCat(1)))));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: multiple calls to 'absl::StrCat' can be flattened into a single call
+
+  // TODO: should trigger. The issue here is that in the current
+  // implementation we ignore any StrCat with StrCat ancestors. Therefore
+  // inserting anything in between calls will disable triggering the deepest
+  // ones.
+  // s = StrCat(Identity(StrCat(StrCat(1, 2), StrCat(3, 4))));
+
+  StrAppend(&S, 001, StrCat(1, 2, "3"), StrCat("FOO"));
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple calls to 'absl::StrCat' can be flattened into a single call
+
+  StrAppend(&S, 001, StrCat(StrCat(1, 2), "3"), StrCat("FOO"));
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple calls to 'absl::StrCat' can be flattened into a single call
+
+  // Too many args. Ignore for now.
+  S = StrCat(1, 2, StrCat(3, 4, 5, 6, 7), 8, 9, 10,
+             StrCat(11, 12, 13, 14, 15, 16, 17, 18), 19, 20, 21, 22, 23, 24, 25,
+             26, 27);
+  // CHECK-MESSAGES: :[[@LINE-3]]:7: warning: multiple calls to 'absl::StrCat' can be flattened into a single call
+  StrAppend(&S, StrCat(1, 2, 3, 4, 5), StrCat(6, 7, 8, 9, 10));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: multiple calls to 'absl::StrCat' can be flattened into a single call
+}
+
+void Negatives() {
+  // One arg. It is used for conversion. Ignore.
+  string S = StrCat(1);
+
+#define A_MACRO(x, y, z) StrCat(x, y, z)
+  S = A_MACRO(1, 2, StrCat("A", "B"));
+}