Index: clang-tidy/abseil/AbseilTidyModule.cpp =================================================================== --- clang-tidy/abseil/AbseilTidyModule.cpp +++ clang-tidy/abseil/AbseilTidyModule.cpp @@ -12,6 +12,7 @@ #include "../ClangTidyModuleRegistry.h" #include "DurationDivisionCheck.h" #include "FasterStrsplitDelimiterCheck.h" +#include "RedundantStrcatCallsCheck.h" #include "StringFindStartswithCheck.h" namespace clang { @@ -25,6 +26,8 @@ "abseil-duration-division"); CheckFactories.registerCheck( "abseil-faster-strsplit-delimiter"); + 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 @@ -4,6 +4,7 @@ AbseilTidyModule.cpp DurationDivisionCheck.cpp FasterStrsplitDelimiterCheck.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,142 @@ +//===--- 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 call_to_strcat = + callExpr(callee(functionDecl(hasName("::absl::StrCat")))); + const auto call_to_strappend = + 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 call_to_either = callExpr( + callee(functionDecl(hasAnyName("::absl::StrCat", "::absl::StrAppend")))); + Finder->addMatcher( + callExpr(call_to_strcat, unless(hasAncestor(call_to_either))) + .bind("StrCat"), + this); + Finder->addMatcher(call_to_strappend.bind("StrAppend"), this); +} + +namespace { + +struct StrCatCheckResult { + int num_calls = 0; + std::vector hints; +}; + +void RemoveCallLeaveArgs(const CallExpr* call, + StrCatCheckResult* check_result) { + // Remove 'Foo(' + check_result->hints.push_back( + FixItHint::CreateRemoval(CharSourceRange::getCharRange( + call->getBeginLoc(), call->getArg(0)->getBeginLoc()))); + // Remove the ')' + check_result->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* check_result) { + const auto is_alphanum = hasDeclaration(cxxMethodDecl(hasName("AlphaNum"))); + static const auto* const str_cat = new auto(hasName("::absl::StrCat")); + const auto is_strcat = cxxBindTemporaryExpr( + has(callExpr(callee(functionDecl(*str_cat))).bind("StrCat"))); + if (const auto* sub_strcat_call = selectFirst( + "StrCat", + match(stmt(anyOf( + cxxConstructExpr(is_alphanum, hasArgument(0, is_strcat)), + is_strcat)), + *arg->IgnoreParenImpCasts(), *Result.Context))) { + RemoveCallLeaveArgs(sub_strcat_call, check_result); + return sub_strcat_call; + } + return nullptr; +} + +StrCatCheckResult ProcessCall(const CallExpr* root_call, bool is_append, + const MatchFinder::MatchResult& Result) { + StrCatCheckResult check_result; + std::deque calls_to_process = {root_call}; + + while (!calls_to_process.empty()) { + ++check_result.num_calls; + + const CallExpr* call_expr = calls_to_process.front(); + calls_to_process.pop_front(); + + int start_arg = call_expr == root_call && is_append; + for (const auto arg : call_expr->arguments()) { + if (start_arg-- > 0) + continue; + if (const clang::CallExpr* sub = + ProcessArgument(arg, Result, &check_result)) { + calls_to_process.push_back(sub); + } + } + } + return check_result; +} +} // namespace + +void RedundantStrcatCallsCheck::check(const MatchFinder::MatchResult& Result) { + bool is_append; + + const CallExpr* root_call; + if ((root_call = Result.Nodes.getNodeAs("StrCat"))) { + is_append = false; + } else if ((root_call = Result.Nodes.getNodeAs("StrAppend"))) { + is_append = true; + } else { + return; + } + + if (root_call->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 check_result = + ProcessCall(root_call, is_append, Result); + if (check_result.num_calls == 1) { + // Just one call, so nothing to fix. + return; + } + + diag(root_call->getBeginLoc(), "redundant StrCat calls") + << check_result.hints; +} + +} // namespace abseil +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -69,7 +69,13 @@ 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-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,20 @@ +.. 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 ``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++ + + string s = StrCat("A", StrCat("B", StrCat("C", "D"))); + ==> string s = StrCat("A", "B", "C", "D"); + StrAppend(&s, StrCat("E", "F", "G")); + ==> StrAppend(&s, "E", "F", "G"); + \ No newline at end of file Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -6,6 +6,7 @@ .. toctree:: abseil-duration-division abseil-faster-strsplit-delimiter + 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: redundant StrCat calls + + S = StrCat(StrCat(StrCat(StrCat(StrCat(1))))); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: redundant StrCat calls + + // 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: redundant StrCat calls + + StrAppend(&S, 001, StrCat(StrCat(1, 2), "3"), StrCat("FOO")); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: redundant StrCat calls + + // 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: redundant StrCat calls + StrAppend(&S, StrCat(1, 2, 3, 4, 5), StrCat(6, 7, 8, 9, 10)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant StrCat calls +} + +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")); +}