[PATCH] D16152: [clang-tidy] Add check performance-faster-string-find

[Alexander Kornienko via cfe-commits]

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