Index: clang-tidy/abseil/AbseilTidyModule.cpp =================================================================== --- clang-tidy/abseil/AbseilTidyModule.cpp +++ clang-tidy/abseil/AbseilTidyModule.cpp @@ -13,6 +13,7 @@ #include "DurationDivisionCheck.h" #include "FasterStrsplitDelimiterCheck.h" #include "NoNamespaceCheck.h" +#include "RedundantStrcatCallsCheck.h" #include "StringFindStartswithCheck.h" namespace clang { @@ -27,6 +28,8 @@ CheckFactories.registerCheck( "abseil-faster-strsplit-delimiter"); CheckFactories.registerCheck("abseil-no-namespace"); + CheckFactories.registerCheck( + "abseil-redundant-strcat-calls"); CheckFactories.registerCheck( "abseil-string-find-startswith"); } Index: clang-tidy/abseil/CMakeLists.txt =================================================================== --- clang-tidy/abseil/CMakeLists.txt +++ clang-tidy/abseil/CMakeLists.txt @@ -5,6 +5,7 @@ DurationDivisionCheck.cpp FasterStrsplitDelimiterCheck.cpp NoNamespaceCheck.cpp + RedundantStrcatCallsCheck.cpp StringFindStartswithCheck.cpp LINK_LIBS Index: clang-tidy/abseil/RedundantStrcatCallsCheck.h =================================================================== --- clang-tidy/abseil/RedundantStrcatCallsCheck.h +++ clang-tidy/abseil/RedundantStrcatCallsCheck.h @@ -0,0 +1,39 @@ +//===--- 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 Index: clang-tidy/abseil/RedundantStrcatCallsCheck.cpp =================================================================== --- clang-tidy/abseil/RedundantStrcatCallsCheck.cpp +++ clang-tidy/abseil/RedundantStrcatCallsCheck.cpp @@ -0,0 +1,140 @@ +//===--- 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 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( + "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 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("StrCat"))) + IsAppend = false; + else if ((RootCall = Result.Nodes.getNodeAs("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 Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -71,11 +71,17 @@ delimiter is a single character string literal and replaces with a character. - New :doc:`abseil-no-namespace - ` check. + ` check. Ensures code does not open ``namespace absl`` as that violates Abseil's compatibility guidelines. +- New :doc:`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 ` check. Index: docs/clang-tidy/checks/abseil-redundant-strcat-calls.rst =================================================================== --- docs/clang-tidy/checks/abseil-redundant-strcat-calls.rst +++ docs/clang-tidy/checks/abseil-redundant-strcat-calls.rst @@ -0,0 +1,26 @@ +.. 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 Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -7,6 +7,7 @@ abseil-duration-division abseil-faster-strsplit-delimiter abseil-no-namespace + abseil-redundant-strcat-calls abseil-string-find-startswith android-cloexec-accept android-cloexec-accept4 Index: test/clang-tidy/abseil-redundant-strcat-calls.cpp =================================================================== --- test/clang-tidy/abseil-redundant-strcat-calls.cpp +++ test/clang-tidy/abseil-redundant-strcat-calls.cpp @@ -0,0 +1,188 @@ +// 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 +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 +class char_traits {}; +template +class allocator {}; +} // namespace std + +template , + typename C = std::allocator> +class basic_string : public __gnu_cxx::__versa_string { + 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 +basic_string operator+(const basic_string &, + const basic_string &); +template +basic_string operator+(const basic_string &, const char *); + +typedef basic_string 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 _Alloc = allocator<_CharT>> +class basic_string; + +template +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 string; +} // namespace std + +namespace absl { + +class string_view { + public: + typedef std::char_traits traits_type; + + string_view(); + string_view(const char *); + string_view(const string &); + string_view(const char *, int); + string_view(string_view, int); + + template + explicit operator ::basic_string() 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 +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 +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")); +}