[clang-tidy] Abseil: faster strsplit delimiter check

[deannagarcia]

Summary

This check is an abseil specific check that checks for code using single character string literals as delimiters and transforms the code into characters.

The check was developed internally and has been running in google3 code, this is just a move to open source the check. It was originally written by @sbenza

[JonasToth]

clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp

25

you dont need the ast_matchers namespace as there is a using namespace ast_matchers at the top, same at other places.

50

The comment suggest, that all single quotes need to be escaped and then further processing happens, but you check on identity to ' and return the escaped version of it.
That confuses me.

55

This expects at max 2 " characters. couldnt there be more?

72

Please make the comments full sentences

76

What about the std::string_view?

109

Please dont use auto here

119

You can configure this diagnostic with %select{option1|option2}.

See https://clang.llvm.org/docs/InternalsManual.html#formatting-a-diagnostic-argument
or bugprone/ForwardingReferenceOverloadCheck.cpp line 133 as an example (there are of course other places that technique is used)

120

This diagnostic is really long, maybe you can shorten it a bit?

E.g. consider using character overload for the second part
and .. called with single character string literal;

My diagnostics are usually not so good, maybe you come up with something better :)

clang-tidy/abseil/FasterStrsplitDelimiterCheck.h

21

s/replaces with/replaces it with/

docs/ReleaseNotes.rst

63

Please highlight code with two `

docs/clang-tidy/checks/abseil-faster-strsplit-delimiter.rst

9

I think:
It will also catch code using ...

sounds a little better

test/clang-tidy/abseil-faster-strsplit-delimiter.cpp

43

Please add a test, where "A" is used as an arbitray function argument (similar to the template case, but without templates involved)

[Eugene.Zelenko]

docs/clang-tidy/checks/abseil-faster-strsplit-delimiter.rst

7

Please make first statement same as in Release Notes. Please also avoid this check.

22

Please separate before and after with empty line. Same in other places.

[hokein]

Thanks for porting the check to upstream (context: the check was developed internally, and has been run on our codebase for a while, it is pretty stable). Could you please update the patch message to indicate this is a porting change, and add the original check author @sbenza in the patch description?

[deannagarcia]

clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp

50

Does the new comment clear it up?

55

The only way this will be run is if the string is a single character, so the only possibility if there are more than 2 " characters is that the character that is the delimiter is actually " which is taken care of in the check. Does that make sense/do you think I need to add a comment about this?

76

This matcher is only here for the next matcher to check to see if absl::ByAnyChar has been passed in a single character string view and is only necessary because ByAnyChar accepts an absl::string_view so it isn't necessary to make one for std::string_view

test/clang-tidy/abseil-faster-strsplit-delimiter.cpp

43

Can you explain this more/provide an example?

[JonasToth]

clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp

50

Yes, thank you.

55

Ok I see, you could add an assertion before this section. Having the precondition checked is always a good thing and usually adds clarity as well :)

76

Ok.

86

Maybe you could make these comments into full sentences. I do think its clear what they mean, but the rule is full sentences.

Maybe Find uses of 'absl::...' to transform them into 'absl::...'.?

116

You can ellide the braces in this case.

test/clang-tidy/abseil-faster-strsplit-delimiter.cpp

43

The testcase i had in mind does not make sense, sorry for noise.

[deannagarcia]

clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp

55

I can't think of a simple thing to assert because most literals will look like: "a" but there is also the possibility that it looks like "\"" and I can't think of an easy way to combine those two. Do you have an idea/maybe I could just put a comment at the beginning saying this is only meant for single character string literals?

[JonasToth]

clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp

55

Wouldnt that result in an assert approximatly this:

assert(Result.size() == 1 || (Result.size() == 2 && Result.startswith('\\'))

Stating the size constraint alone is already a valuable assert in my opinion.

86

These comments seem to be longer then 80, if true please wrap them.

[hokein]

The patch looks good. I'll commit for you once @JonasToth has no further comments.

[JonasToth]

LGTM

[hokein]

Committed in rL340411.

clang-tidy/abseil/AbseilTidyModule.cpp

 //===------- AbseilTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "FasterStrsplitDelimiterCheck.h"
 #include "StringFindStartswithCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace abseil {
 
 class AbseilModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<FasterStrsplitDelimiterCheck>(
+        "abseil-faster-strsplit-delimiter");
     CheckFactories.registerCheck<StringFindStartswithCheck>(
         "abseil-string-find-startswith");
   }
 };
 
 // Register the AbseilModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<AbseilModule> X("abseil-module",
                                                     "Add Abseil checks.");

clang-tidy/abseil/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
+  FasterStrsplitDelimiterCheck.cpp
   StringFindStartswithCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tidy/abseil/FasterStrsplitDelimiterCheck.h

+//===--- 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

clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp

+//===--- 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<Expr>
+constructExprWithArg(llvm::StringRef ClassName,
+                     const ::internal::Matcher<Expr> &Arg) {
+  auto ConstrExpr = cxxConstructExpr(hasType(recordDecl(hasName(ClassName))),
+                                     hasArgument(0, ignoringParenCasts(Arg)));
+
+  return anyOf(ConstrExpr, cxxBindTemporaryExpr(has(ConstrExpr)));
+}
+
+::internal::Matcher<Expr>
+copyConstructExprWithArg(llvm::StringRef ClassName,
+                         const ::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 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<StringLiteral>("Literal");
+
+  if (Literal->getBeginLoc().isMacroID() || Literal->getEndLoc().isMacroID())
+    return;
+
+  llvm::Optional<std::string> 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(),
+      "%select{absl::StrSplit()|absl::MaxSplits()}0 called with a string "
+      "literal "
+      "consisting of a single character; consider using the character overload")
+      << (Result.Nodes.getNodeAs<CallExpr>("StrSplit") ? 0 : 1)
+      << FixItHint::CreateReplacement(CharSourceRange::getTokenRange(Range),
+                                      *Replacement);
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 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.
 
   Detects usage of magic numbers, numbers that are used as literals instead of
   introduced via constants or symbols.
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

docs/clang-tidy/checks/abseil-faster-strsplit-delimiter.rst

+.. 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))) {

docs/clang-tidy/checks/list.rst

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
+   abseil-faster-strsplit-delimiter
    abseil-string-find-startswith
    android-cloexec-accept
    android-cloexec-accept4
    android-cloexec-creat
    android-cloexec-dup
    android-cloexec-epoll-create
    android-cloexec-epoll-create1
    android-cloexec-fopen

test/clang-tidy/abseil-faster-strsplit-delimiter.cpp

+// 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() 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 <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()
+  // CHECK-FIXES: absl::StrSplit("TTT", 'A');
+}
+
+void FunctionTemplateCaller() {
+  FunctionTemplate<absl::ByAnyChar>();
+  FunctionTemplate<absl::string_view>();
+}