Enhance abseil-faster-strsplit-delimiter to handle other non-printable characters.

[zhangxy988]

Summary

Currently it fails on cases like '\001'.

Note: Since StringLiteral::outputString dumps most nonprintable
characters in octal value, the exact string literal format isn't preserved,
e.g. "\x01" becomes '\001'.

[gribozavr]

Do you have commit access? Would you like me to commit this patch for you?

[zhangxy988]

I don't think I have commit access.
Please commit it for me. Thanks!

[gribozavr]

Sorry, the patch does not apply cleanly to current master -- could you rebase please?

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp

10 ↗

(On Diff #207853)

Unnecessary empty line.

[lebedev.ri]

clang-tools-extra/clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp

42–43 ↗

(On Diff #207853)

Assertion messages missing

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

55 ↗

(On Diff #207853)

Is there a negative test, absl::StrSplit("ABC", R"(AA)");?
Also what about wide chars? (the second assertion)

[zhangxy988]

• Remove empty line and add assertion message.

[zhangxy988]

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

55 ↗

(On Diff #207853)

I don't really know how to write a negative test like that.
This check is only concerned with single character string literal and it doesn't support wide char (since StrSplit doesn't).

[lebedev.ri]

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

55 ↗

(On Diff #207853)

What i'm asking is, those assertions are there just to say "look, i guarantee those invariants hold here",
or to say "oh no, we shouldn't have ever violated these invariants here"? The difference being, in former case,
there is some other error checking that prevents those assertions from triggering; in latter case there is not.

[gribozavr]

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

55 ↗

(On Diff #207853)

There's a test below for multi-character string literals, absl::StrSplit("ABC", "AB");.

As far as wide characters go, code with wide characters won't pass semantic analysis because there's no StrSplit overload that accepts wide characters.

[lebedev.ri]

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

55 ↗

(On Diff #207853)

Cool, i have no further comments here then, thanks.

clang-tools-extra/trunk/clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp

 //===--- FasterStrsplitDelimiterCheck.cpp - clang-tidy---------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "FasterStrsplitDelimiterCheck.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/FixIt.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace abseil {
 
 namespace {
 
 AST_MATCHER(StringLiteral, lengthIsOne) { return Node.getLength() == 1; }
 
-llvm::Optional<std::string> makeCharacterLiteral(const StringLiteral *Literal) {
-  std::string Result;
-  {
+llvm::Optional<std::string> makeCharacterLiteral(const StringLiteral *Literal,
+                                                 const ASTContext &Context) {
+  assert(Literal->getLength() == 1 &&
+         "Only single character string should be matched");
+  assert(Literal->getCharByteWidth() == 1 &&
+         "StrSplit doesn't support wide char");
+  std::string Result = clang::tooling::fixit::getText(*Literal, Context).str();
+  bool IsRawStringLiteral = StringRef(Result).startswith(R"(R")");
+  // Since raw string literal might contain unescaped non-printable characters,
+  // we normalize them using `StringLiteral::outputString`.
+  if (IsRawStringLiteral) {
+    Result.clear();
     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('"');
+  std::string::size_type 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;

 
 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);
+  llvm::Optional<std::string> Replacement =
+      makeCharacterLiteral(Literal, *Result.Context);
   if (!Replacement)
     return;
   SourceRange Range = Literal->getSourceRange();
 
   if (const auto *ByAnyChar = Result.Nodes.getNodeAs<Expr>("ByAnyChar"))
     Range = ByAnyChar->getSourceRange();
 
   diag(

clang-tools-extra/trunk/test/clang-tidy/abseil-faster-strsplit-delimiter.cpp

 
 } //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", "\x01");
+  // 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", '\x01');
+
+  absl::StrSplit("ABC", "\001");
+  // 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", '\001');
+
+  absl::StrSplit("ABC", R"(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", R"(')");
+  // 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", '\'');
+
+  absl::StrSplit("ABC", R"(
+)");
+  // CHECK-MESSAGES: [[@LINE-2]]: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", '\n');
+
+  absl::StrSplit("ABC", R"delimiter(A)delimiter");
+  // 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; });