diff --git a/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp b/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp index 061fc7c4d137..c4f27a492867 100644 --- a/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp @@ -11,6 +11,7 @@ #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" #include "DurationDivisionCheck.h" +#include "FasterStrsplitDelimiterCheck.h" #include "StringFindStartswithCheck.h" namespace clang { @@ -22,6 +23,8 @@ public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { CheckFactories.registerCheck( "abseil-duration-division"); + CheckFactories.registerCheck( + "abseil-faster-strsplit-delimiter"); CheckFactories.registerCheck( "abseil-string-find-startswith"); } diff --git a/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt b/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt index a2478fd428e7..0a2e84b63c62 100644 --- a/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt @@ -3,6 +3,7 @@ set(LLVM_LINK_COMPONENTS support) add_clang_library(clangTidyAbseilModule AbseilTidyModule.cpp DurationDivisionCheck.cpp + FasterStrsplitDelimiterCheck.cpp StringFindStartswithCheck.cpp LINK_LIBS diff --git a/clang-tools-extra/clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp b/clang-tools-extra/clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp new file mode 100644 index 000000000000..35d99ac18822 --- /dev/null +++ b/clang-tools-extra/clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp @@ -0,0 +1,131 @@ +//===--- FasterStrsplitDelimiterCheck.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 "FasterStrsplitDelimiterCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace abseil { + +namespace { + +AST_MATCHER(StringLiteral, lengthIsOne) { return Node.getLength() == 1; } + +::internal::Matcher +constructExprWithArg(llvm::StringRef ClassName, + const ::internal::Matcher &Arg) { + auto ConstrExpr = cxxConstructExpr(hasType(recordDecl(hasName(ClassName))), + hasArgument(0, ignoringParenCasts(Arg))); + + return anyOf(ConstrExpr, cxxBindTemporaryExpr(has(ConstrExpr))); +} + +::internal::Matcher +copyConstructExprWithArg(llvm::StringRef ClassName, + const ::internal::Matcher &Arg) { + return constructExprWithArg(ClassName, constructExprWithArg(ClassName, Arg)); +} + +llvm::Optional makeCharacterLiteral(const StringLiteral *Literal) { + std::string Result; + { + llvm::raw_string_ostream Stream(Result); + Literal->outputString(Stream); + } + + // Special case: If the string contains a single quote, we just need to return + // a character of the single quote. This is a special case because we need to + // escape it in the character literal. + if (Result == R"("'")") + return std::string(R"('\'')"); + + assert(Result.size() == 3 || (Result.size() == 4 && Result.substr(0, 2) == "\"\\")); + + // Now replace the " with '. + auto Pos = Result.find_first_of('"'); + if (Pos == Result.npos) + return llvm::None; + Result[Pos] = '\''; + Pos = Result.find_last_of('"'); + if (Pos == Result.npos) + return llvm::None; + Result[Pos] = '\''; + return Result; +} + +} // anonymous namespace + +void FasterStrsplitDelimiterCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus) + return; + + // Binds to one character string literals. + const auto SingleChar = + expr(ignoringParenCasts(stringLiteral(lengthIsOne()).bind("Literal"))); + + // Binds to a string_view (either absl or std) that was passed by value and + // contructed from string literal. + auto StringViewArg = + copyConstructExprWithArg("::absl::string_view", SingleChar); + + auto ByAnyCharArg = + expr(copyConstructExprWithArg("::absl::ByAnyChar", StringViewArg)) + .bind("ByAnyChar"); + + // Find uses of absl::StrSplit(..., "x") and absl::StrSplit(..., + // absl::ByAnyChar("x")) to transform them into absl::StrSplit(..., 'x'). + Finder->addMatcher(callExpr(callee(functionDecl(hasName("::absl::StrSplit"))), + hasArgument(1, anyOf(ByAnyCharArg, SingleChar)), + unless(isInTemplateInstantiation())) + .bind("StrSplit"), + this); + + // Find uses of absl::MaxSplits("x", N) and + // absl::MaxSplits(absl::ByAnyChar("x"), N) to transform them into + // absl::MaxSplits('x', N). + Finder->addMatcher( + callExpr( + callee(functionDecl(hasName("::absl::MaxSplits"))), + hasArgument(0, anyOf(ByAnyCharArg, ignoringParenCasts(SingleChar))), + unless(isInTemplateInstantiation())), + this); +} + +void FasterStrsplitDelimiterCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *Literal = Result.Nodes.getNodeAs("Literal"); + + if (Literal->getBeginLoc().isMacroID() || Literal->getEndLoc().isMacroID()) + return; + + llvm::Optional Replacement = makeCharacterLiteral(Literal); + if (!Replacement) + return; + SourceRange Range = Literal->getSourceRange(); + + if (const auto *ByAnyChar = Result.Nodes.getNodeAs("ByAnyChar")) + Range = ByAnyChar->getSourceRange(); + + diag( + Literal->getBeginLoc(), + "%select{absl::StrSplit()|absl::MaxSplits()}0 called with a string " + "literal " + "consisting of a single character; consider using the character overload") + << (Result.Nodes.getNodeAs("StrSplit") ? 0 : 1) + << FixItHint::CreateReplacement(CharSourceRange::getTokenRange(Range), + *Replacement); +} + +} // namespace abseil +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/abseil/FasterStrsplitDelimiterCheck.h b/clang-tools-extra/clang-tidy/abseil/FasterStrsplitDelimiterCheck.h new file mode 100644 index 000000000000..17e231cb1621 --- /dev/null +++ b/clang-tools-extra/clang-tidy/abseil/FasterStrsplitDelimiterCheck.h @@ -0,0 +1,36 @@ +//===--- FasterStrsplitDelimiterCheck.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_FASTERSTRSPLITDELIMITERCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_FASTERSTRSPLITDELIMITERCHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace abseil { + +/// Finds instances of absl::StrSplit() or absl::MaxSplits() where the delimiter +/// is a single character string literal and replaces it with a character. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-faster-strsplit-delimiter.html +class FasterStrsplitDelimiterCheck : public ClangTidyCheck { +public: + FasterStrsplitDelimiterCheck(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_FASTERSTRSPLITDELIMITERCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index cea98b334e33..3d9389d82e83 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -64,6 +64,12 @@ Improvements to clang-tidy floating-point context, and recommends the use of a function that returns a floating-point value. +- New :doc:`abseil-faster-strsplit-delimiter + ` check. + + Finds instances of ``absl::StrSplit()`` or ``absl::MaxSplits()`` where the + delimiter is a single character string literal and replaces with a character. + - New :doc:`readability-magic-numbers ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/abseil-faster-strsplit-delimiter.rst b/clang-tools-extra/docs/clang-tidy/checks/abseil-faster-strsplit-delimiter.rst new file mode 100644 index 000000000000..fe9115652b53 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/abseil-faster-strsplit-delimiter.rst @@ -0,0 +1,41 @@ +.. title:: clang-tidy - abseil-faster-strsplit-delimiter + +abseil-faster-strsplit-delimiter +================================ + +Finds instances of ``absl::StrSplit()`` or ``absl::MaxSplits()`` where the +delimiter is a single character string literal and replaces with a character. +The check will offer a suggestion to change the string literal into a character. +It will also catch code using ``absl::ByAnyChar()`` for just a single character +and will transform that into a single character as well. + +These changes will give the same result, but using characters rather than +single character string literals is more efficient and readable. + +Examples: + +.. code-block:: c++ + + // Original - the argument is a string literal. + for (auto piece : absl::StrSplit(str, "B")) { + + // Suggested - the argument is a character, which causes the more efficient + // overload of absl::StrSplit() to be used. + for (auto piece : absl::StrSplit(str, 'B')) { + + + // Original - the argument is a string literal inside absl::ByAnyChar call. + for (auto piece : absl::StrSplit(str, absl::ByAnyChar("B"))) { + + // Suggested - the argument is a character, which causes the more efficient + // overload of absl::StrSplit() to be used and we do not need absl::ByAnyChar + // anymore. + for (auto piece : absl::StrSplit(str, 'B')) { + + + // Original - the argument is a string literal inside absl::MaxSplits call. + for (auto piece : absl::StrSplit(str, absl::MaxSplits("B", 1))) { + + // Suggested - the argument is a character, which causes the more efficient + // overload of absl::StrSplit() to be used. + for (auto piece : absl::StrSplit(str, absl::MaxSplits('B', 1))) { diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 696905b50d2d..3ecb17c254e4 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -5,6 +5,7 @@ Clang-Tidy Checks .. toctree:: abseil-duration-division + abseil-faster-strsplit-delimiter abseil-string-find-startswith android-cloexec-accept android-cloexec-accept4 diff --git a/clang-tools-extra/test/clang-tidy/abseil-faster-strsplit-delimiter.cpp b/clang-tools-extra/test/clang-tidy/abseil-faster-strsplit-delimiter.cpp new file mode 100644 index 000000000000..5895e45a9c7e --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/abseil-faster-strsplit-delimiter.cpp @@ -0,0 +1,99 @@ +// RUN: %check_clang_tidy %s abseil-faster-strsplit-delimiter %t + +namespace absl { + +class string_view { + public: + string_view(); + string_view(const char *); +}; + +namespace strings_internal { +struct Splitter {}; +struct MaxSplitsImpl { + MaxSplitsImpl(); + ~MaxSplitsImpl(); +}; +} //namespace strings_internal + +template +strings_internal::Splitter StrSplit(absl::string_view, Delim) { + return {}; +} +template +strings_internal::Splitter StrSplit(absl::string_view, Delim, Pred) { + return {}; +} + +class ByAnyChar { + public: + explicit ByAnyChar(absl::string_view); + ~ByAnyChar(); +}; + +template +strings_internal::MaxSplitsImpl MaxSplits(Delim, int) { + return {}; +} + +} //namespace absl + +void SplitDelimiters() { + absl::StrSplit("ABC", "A"); + // CHECK-MESSAGES: [[@LINE-1]]:25: warning: absl::StrSplit() called with a string literal consisting of a single character; consider using the character overload [abseil-faster-strsplit-delimiter] + // CHECK-FIXES: absl::StrSplit("ABC", 'A'); + + absl::StrSplit("ABC", absl::ByAnyChar("\n")); + // CHECK-MESSAGES: [[@LINE-1]]:41: warning: absl::StrSplit() + // CHECK-FIXES: absl::StrSplit("ABC", '\n'); + + // Works with predicate + absl::StrSplit("ABC", "A", [](absl::string_view) { return true; }); + // CHECK-MESSAGES: [[@LINE-1]]:25: warning: absl::StrSplit() + // CHECK-FIXES: absl::StrSplit("ABC", 'A', [](absl::string_view) { return true; }); + + // Doesn't do anything with other strings lenghts. + absl::StrSplit("ABC", "AB"); + absl::StrSplit("ABC", absl::ByAnyChar("")); + absl::StrSplit("ABC", absl::ByAnyChar(" \t")); + + // Escapes a single quote in the resulting character literal. + absl::StrSplit("ABC", "'"); + // CHECK-MESSAGES: [[@LINE-1]]:25: warning: absl::StrSplit() + // CHECK-FIXES: absl::StrSplit("ABC", '\''); + + absl::StrSplit("ABC", "\""); + // CHECK-MESSAGES: [[@LINE-1]]:25: warning: absl::StrSplit() + // CHECK-FIXES: absl::StrSplit("ABC", '\"'); + + absl::StrSplit("ABC", absl::MaxSplits("\t", 1)); + // CHECK-MESSAGES: [[@LINE-1]]:41: warning: absl::MaxSplits() + // CHECK-FIXES: absl::StrSplit("ABC", absl::MaxSplits('\t', 1)); + + auto delim = absl::MaxSplits(absl::ByAnyChar(" "), 1); + // CHECK-MESSAGES: [[@LINE-1]]:48: warning: absl::MaxSplits() + // CHECK-FIXES: auto delim = absl::MaxSplits(' ', 1); +} + +#define MACRO(str) absl::StrSplit("ABC", str) + +void Macro() { + MACRO("A"); +} + +template +void FunctionTemplate() { + // This one should not warn because ByAnyChar is a dependent type. + absl::StrSplit("TTT", T("A")); + + // This one will warn, but we are checking that we get a correct warning only + // once. + absl::StrSplit("TTT", "A"); + // CHECK-MESSAGES: [[@LINE-1]]:25: warning: absl::StrSplit() + // CHECK-FIXES: absl::StrSplit("TTT", 'A'); +} + +void FunctionTemplateCaller() { + FunctionTemplate(); + FunctionTemplate(); +}