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