deannagarcia created this revision.
deannagarcia added reviewers: hokein, alexfh.
deannagarcia added a project: clang-tools-extra.
Herald added subscribers: xazax.hun, mgorny.
This check is an abseil specific check that checks for code using single
character string literals as delimiters and transforms the code into characters.
https://reviews.llvm.org/D50862
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp
clang-tidy/abseil/FasterStrsplitDelimiterCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/abseil-faster-strsplit-delimiter.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/abseil-faster-strsplit-delimiter.cpp
Index: test/clang-tidy/abseil-faster-strsplit-delimiter.cpp
===================================================================
--- test/clang-tidy/abseil-faster-strsplit-delimiter.cpp
+++ test/clang-tidy/abseil-faster-strsplit-delimiter.cpp
@@ -0,0 +1,95 @@
+// 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 <typename Delim>
+strings_internal::Splitter StrSplit(absl::string_view, Delim) {
+ return {};
+}
+template <typename Delim, typename Pred>
+strings_internal::Splitter StrSplit(absl::string_view, Delim, Pred) {
+ return {};
+}
+
+class ByAnyChar {
+ public:
+ explicit ByAnyChar(absl::string_view);
+ ~ByAnyChar();
+};
+
+template <typename Delim>
+strings_internal::MaxSplitsImpl MaxSplits(Delim, int) {
+ return {};
+}
+
+} //namespace absl
+
+void SplitDelimiters() {
+ absl::StrSplit("ABC", "A");
+ // CHECK-MESSAGES: [[@LINE-1]]:25: warning: absl::StrSplit()/ByAnyChar()/MaxSplits() called with a string literal consisting of a single character; consider using the more efficient overload accepting a character [abseil-faster-strsplit-delimiter]
+ // CHECK-FIXES: absl::StrSplit("ABC", 'A');
+
+ absl::StrSplit("ABC", absl::ByAnyChar("\n"));
+ // CHECK-MESSAGES: [[@LINE-1]]:41: warning: absl::StrSplit()/ByAnyChar()
+ // 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()/ByAnyChar()
+ // 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()/ByAnyChar()
+ // CHECK-FIXES: absl::StrSplit("ABC", '\'');
+
+ absl::StrSplit("ABC", absl::MaxSplits("\t", 1));
+ // CHECK-MESSAGES: [[@LINE-1]]:41: warning: absl::StrSplit()/ByAnyChar()
+ // CHECK-FIXES: absl::StrSplit("ABC", absl::MaxSplits('\t', 1));
+
+ auto delim = absl::MaxSplits(absl::ByAnyChar(" "), 1);
+ // CHECK-MESSAGES: [[@LINE-1]]:48: warning: absl::StrSplit()/ByAnyChar()
+ // CHECK-FIXES: auto delim = absl::MaxSplits(' ', 1);
+}
+
+#define MACRO(str) absl::StrSplit("ABC", str)
+
+void Macro() {
+ MACRO("A");
+}
+
+template <typename T>
+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()/ByAnyChar()
+ // CHECK-FIXES: absl::StrSplit("TTT", 'A');
+}
+
+void FunctionTemplateCaller() {
+ FunctionTemplate<absl::ByAnyChar>();
+ FunctionTemplate<absl::string_view>();
+}
Index: docs/clang-tidy/checks/list.rst
===================================================================
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -4,6 +4,7 @@
=================
.. toctree::
+ abseil-faster-strsplit-delimiter
abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
Index: docs/clang-tidy/checks/abseil-faster-strsplit-delimiter.rst
===================================================================
--- docs/clang-tidy/checks/abseil-faster-strsplit-delimiter.rst
+++ docs/clang-tidy/checks/abseil-faster-strsplit-delimiter.rst
@@ -0,0 +1,36 @@
+.. title:: clang-tidy - abseil-faster-strsplit-delimiter
+
+abseil-faster-strsplit-delimiter
+================================
+
+This check triggers on calls to ``absl::StrSplit()`` or ``absl::MaxSplits()``
+where the delimiter is a single character string literal. The check will offer
+a suggestion to change the string literal into a character. It will also catch
+when code uses ``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))) {
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,13 @@
Improvements to clang-tidy
--------------------------
+- New :doc:`abseil-faster-strsplit-delimiter
+ <clang-tidy/checks/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
<clang-tidy/checks/readability-magic-numbers>` check.
Index: clang-tidy/abseil/FasterStrsplitDelimiterCheck.h
===================================================================
--- clang-tidy/abseil/FasterStrsplitDelimiterCheck.h
+++ 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 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
Index: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp
===================================================================
--- clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp
+++ clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp
@@ -0,0 +1,127 @@
+//===--- 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; }
+
+ast_matchers::internal::Matcher<Expr>
+constructExprWithArg(llvm::StringRef ClassName,
+ const ast_matchers::internal::Matcher<Expr> &Arg) {
+ auto ConstrExpr = cxxConstructExpr(hasType(recordDecl(hasName(ClassName))),
+ hasArgument(0, ignoringParenCasts(Arg)));
+
+ return anyOf(ConstrExpr, cxxBindTemporaryExpr(has(ConstrExpr)));
+}
+
+ast_matchers::internal::Matcher<Expr>
+copyConstructExprWithArg(llvm::StringRef ClassName,
+ const ast_matchers::internal::Matcher<Expr> &Arg) {
+ return constructExprWithArg(ClassName,
+ constructExprWithArg(ClassName, Arg));
+}
+
+llvm::Optional<std::string> 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 need to escape it
+ // in the character literal.
+ if (Result == R"("'")") {
+ return std::string(R"('\'')");
+ }
+
+ // 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;
+
+ // One character string literal.
+ const auto SingleChar =
+ expr(ignoringParenCasts(stringLiteral(lengthIsOne()).bind("Literal")));
+
+ // string_view passed by value and contructed from string literal.
+ auto StringViewArg =
+ copyConstructExprWithArg("::absl::string_view", SingleChar);
+
+ auto ByAnyCharArg =
+ expr(copyConstructExprWithArg("::absl::ByAnyChar", StringViewArg))
+ .bind("ByAnyChar");
+
+ // absl::StrSplit(..., "x") -> absl::StrSplit(..., 'x')
+ // absl::ByAnyChar("x") -> 'x'
+ Finder->addMatcher(
+ callExpr(callee(functionDecl(hasName("::absl::StrSplit"))),
+ hasArgument(1, anyOf(ByAnyCharArg, SingleChar)),
+ unless(isInTemplateInstantiation())),
+ this);
+
+ // absl::MaxSplits("x", N) -> absl::MaxSplits('x', N)
+ // absl::MaxSplits(absl::ByAnyChar("x"), N) -> 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<StringLiteral>("Literal");
+
+ if (Literal->getBeginLoc().isMacroID() || Literal->getEndLoc().isMacroID())
+ return;
+
+ auto Replacement = makeCharacterLiteral(Literal);
+ if (!Replacement)
+ return;
+ SourceRange Range = Literal->getSourceRange();
+
+ if (const auto *ByAnyChar = Result.Nodes.getNodeAs<Expr>("ByAnyChar")) {
+ Range = ByAnyChar->getSourceRange();
+ }
+
+ diag(Literal->getBeginLoc(),
+ "absl::StrSplit()/ByAnyChar()/MaxSplits() called with a string literal "
+ "consisting of a single character; consider using the more efficient "
+ "overload accepting a character")
+ << FixItHint::CreateReplacement(CharSourceRange::getTokenRange(Range),
+ *Replacement);
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/abseil/CMakeLists.txt
===================================================================
--- clang-tidy/abseil/CMakeLists.txt
+++ clang-tidy/abseil/CMakeLists.txt
@@ -2,6 +2,7 @@
add_clang_library(clangTidyAbseilModule
AbseilTidyModule.cpp
+ FasterStrsplitDelimiterCheck.cpp
StringFindStartswithCheck.cpp
LINK_LIBS
Index: clang-tidy/abseil/AbseilTidyModule.cpp
===================================================================
--- clang-tidy/abseil/AbseilTidyModule.cpp
+++ clang-tidy/abseil/AbseilTidyModule.cpp
@@ -10,6 +10,7 @@
#include "../ClangTidy.h"
#include "../ClangTidyModule.h"
#include "../ClangTidyModuleRegistry.h"
+#include "FasterStrsplitDelimiterCheck.h"
#include "StringFindStartswithCheck.h"
namespace clang {
@@ -19,6 +20,8 @@
class AbseilModule : public ClangTidyModule {
public:
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+ CheckFactories.registerCheck<FasterStrsplitDelimiterCheck>(
+ "abseil-faster-strsplit-delimiter");
CheckFactories.registerCheck<StringFindStartswithCheck>(
"abseil-string-find-startswith");
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
