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