[PATCH] D50862: [clang-tidy] Abseil: faster strsplit delimiter check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:24
+
+ast_matchers::internal::Matcher<Expr>
+constructExprWithArg(llvm::StringRef ClassName,
----------------
you dont need the `ast_matchers` namespace as there is a `using namespace ast_matchers` at the top, same at other places.


================
Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:49
+  // in the character literal.
+  if (Result == R"("'")") {
+    return std::string(R"('\'')");
----------------
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.


================
Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:54
+  // Now replace the " with '.
+  auto Pos = Result.find_first_of('"');
+  if (Pos == Result.npos)
----------------
This expects at max 2 `"` characters. couldnt there be more?


================
Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:71
+
+  // One character string literal.
+  const auto SingleChar =
----------------
Please make the comments full sentences


================
Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:75
+
+  // string_view passed by value and contructed from string literal.
+  auto StringViewArg =
----------------
What about the `std::string_view`?


================
Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:108
+
+  auto Replacement = makeCharacterLiteral(Literal);
+  if (!Replacement)
----------------
Please dont use auto here


================
Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:118
+  diag(Literal->getBeginLoc(),
+       "absl::StrSplit()/ByAnyChar()/MaxSplits() called with a string literal "
+       "consisting of a single character; consider using the more efficient "
----------------
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)


================
Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:119
+       "absl::StrSplit()/ByAnyChar()/MaxSplits() called with a string literal "
+       "consisting of a single character; consider using the more efficient "
+       "overload accepting a character")
----------------
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 :)


================
Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.h:20
+/// Finds instances of absl::StrSplit() or absl::MaxSplits() where the delimiter
+/// is a single character string literal and replaces with a character.
+///
----------------
s/replaces with/replaces it with/


================
Comment at: docs/ReleaseNotes.rst:63
+
+  Finds instances of absl::StrSplit() or absl::MaxSplits() where the delimiter
+  is a single character string literal and replaces with a character.
----------------
Please highlight code with two `


================
Comment at: docs/clang-tidy/checks/abseil-faster-strsplit-delimiter.rst:8
+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
----------------
I think:
It will also catch code using ...

sounds a little better


================
Comment at: test/clang-tidy/abseil-faster-strsplit-delimiter.cpp:42
+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]
----------------
Please add a test, where `"A"` is used as an arbitray function argument (similar to the template case, but without templates involved)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50862