JonasToth added inline comments.
================
Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:85
+
+ // absl::StrSplit(..., "x") -> absl::StrSplit(..., 'x')
+ // absl::ByAnyChar("x") -> 'x'
----------------
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::...'.`?
================
Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:115
+
+ if (const auto *ByAnyChar = Result.Nodes.getNodeAs<Expr>("ByAnyChar")) {
+ Range = ByAnyChar->getSourceRange();
----------------
You can ellide the braces in this case.
================
Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:49
+ // in the character literal.
+ if (Result == R"("'")") {
+ return std::string(R"('\'')");
----------------
deannagarcia wrote:
> JonasToth wrote:
> > 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.
> Does the new comment clear it up?
Yes, thank you.
================
Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:54
+ // Now replace the " with '.
+ auto Pos = Result.find_first_of('"');
+ if (Pos == Result.npos)
----------------
deannagarcia wrote:
> JonasToth wrote:
> > This expects at max 2 `"` characters. couldnt there be more?
> 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?
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 :)
================
Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:75
+
+ // string_view passed by value and contructed from string literal.
+ auto StringViewArg =
----------------
deannagarcia wrote:
> JonasToth wrote:
> > What about the `std::string_view`?
> 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
Ok.
================
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]
----------------
deannagarcia wrote:
> JonasToth wrote:
> > Please add a test, where `"A"` is used as an arbitray function argument
> > (similar to the template case, but without templates involved)
> Can you explain this more/provide an example?
The testcase i had in mind does not make sense, sorry for noise.
https://reviews.llvm.org/D50862
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
