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
