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

Unnecessary empty line.

[lebedev.ri]

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

42–43

Assertion messages missing

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

55

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

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

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

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

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

clang-tools-extra/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 {

 }
 
 ::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::Optional<std::string> makeCharacterLiteral(const StringLiteral *Literal,
+                                                 const ASTContext &Context) {
+  assert(Literal->getLength() == 1);
+  assert(Literal->getCharByteWidth() == 1); // no 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/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", 'B');
+
+  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; });