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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits