alexfh added inline comments. ================ Comment at: clang-tidy/performance/FasterStringFindCheck.cpp:30 @@ +29,3 @@ + Class = Class.trim(); + if (!Class.empty()) + Result.push_back(Class); ---------------- aaron.ballman wrote: > > Also changed the separator to be ';' instead of ','. > > The latter could be part of a type name. Eg. Foo<A,B>::Bar > > That's a great catch! > > @alexfh -- do you think we should try to get our checkers to be consistent > with their choice of list separators? Specifically, I am thinking about > D16113. It seems like it would improve the user experience to not have to > wonder "do I use comma, or semi colon, for this list?" We can try to be consistent with the choice of delimiters, but I'm not sure we can use the same delimiter always without introducing more syntax complexities like escaping or the use of quotes (like in CSV). In any case, we should clearly document the format for each option and think how we can issue diagnostics when we suspect incorrect format used in the option value.
================ Comment at: clang-tidy/performance/FasterStringFindCheck.cpp:72 @@ +71,3 @@ + Opts, "StringLikeClasses", + llvm::join(StringLikeClasses.begin(), StringLikeClasses.end(), ",")); +} ---------------- This has to use the same delimiter. Maybe pull it to a constant to avoid inconsistency? ================ Comment at: clang-tidy/performance/FasterStringFindCheck.h:25 @@ +24,3 @@ +/// The character literal overload is more efficient. +/// +/// For the user-facing documentation see: ---------------- hokein wrote: > I think you need to add document about `StringLikeClasses` option here. Not here, in the .rst file, please. http://reviews.llvm.org/D16152 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
