[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:54
+  // Now replace the " with '.
+  auto Pos = Result.find_first_of('"');
+  if (Pos == Result.npos)
----------------
deannagarcia wrote:
> JonasToth wrote:
> > 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 :)
> I can't think of a simple thing to assert because most literals will look like: "a" but there is also the possibility that it looks like "\"" and I can't think of an easy way to combine those two. Do you have an idea/maybe I could just put a comment at the beginning saying this is only meant for single character string literals?
Wouldnt that result in an assert approximatly this:

`assert(Result.size() == 1 || (Result.size() == 2 && Result.startswith('\\'))`

Stating the size constraint alone is already a valuable assert in my opinion.


================
Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:85
+
+  // Find uses of absl::StrSplit(..., "x") and absl::StrSplit(..., absl::ByAnyChar("x"))
+  // to transform them into absl::StrSplit(..., 'x').
----------------
These comments seem to be longer then 80, if true please wrap them.


https://reviews.llvm.org/D50862