alexfh added a comment. Awesome! See a few comments inline.
================ Comment at: clang-tidy/performance/FasterStringFindCheck.cpp:51 @@ +50,3 @@ + const auto StringFindFunctions = + anyOf(hasName("find"), hasName("rfind"), hasName("find_first_of"), + hasName("find_first_not_of"), hasName("find_last_of"), ---------------- Probably out of scope of this patch, but I wonder how many times `hasName` is still more effective than one `matchesName`? Maybe we should add a `matchesName` variant for unqualified names or hasName taking a list of strings? ================ Comment at: clang-tidy/performance/FasterStringFindCheck.cpp:57 @@ +56,3 @@ + + for (auto &ClassName : StringLikeClasses) { + const auto HasName = hasName(ClassName); ---------------- `const auto &` would be slightly clearer. ================ Comment at: clang-tidy/performance/FasterStringFindCheck.cpp:75 @@ +74,3 @@ + + diag(Literal->getLocStart(), "char overload is more efficient") + << FixItHint::CreateReplacement( ---------------- The message might be confusing in some situations (e.g. macros). I think, it should at least mention what method (and maybe of what class) is in question, e.g. "std::string::find() called with a string literal consisting of a single character; consider using the more effective overload accepting a character". ================ Comment at: clang-tidy/performance/FasterStringFindCheck.h:15 @@ +14,3 @@ + +#include <vector> +#include <string> ---------------- Sort includes, please. ================ Comment at: test/clang-tidy/performance-faster-string-find.cpp:8 @@ +7,3 @@ +template <typename T> +struct basic_string { + int find(const char *, int = 0) const; ---------------- Should we move stubs to a common header(s)? ================ Comment at: test/clang-tidy/performance-faster-string-find.cpp:9 @@ +8,3 @@ +struct basic_string { + int find(const char *, int = 0) const; + int find(const char *, int, int) const; ---------------- Did you mean `const T *`? ================ Comment at: test/clang-tidy/performance-faster-string-find.cpp:31 @@ +30,3 @@ + +void StringFind() { + std::string Str; ---------------- As usual, please add tests with templates and macros ;) ================ Comment at: test/clang-tidy/performance-faster-string-find.cpp:32 @@ +31,3 @@ +void StringFind() { + std::string Str; + ---------------- Please add tests for std::wstring, std::u16string and std::u32string. At the very least, we should ensure we don't break the code using them. ================ Comment at: test/clang-tidy/performance-faster-string-find.cpp:50 @@ +49,3 @@ + + // Some other overloads + Str.rfind("a"); ---------------- "Some other methods." maybe? http://reviews.llvm.org/D16152 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits