[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:1
+#include "StringFindStartswithCheck.h"
+
----------------
nit: We need a LICENSE comment at the top of the file.


================
Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:7
+
+using namespace clang::ast_matchers; // NOLINT: Too many names.
+
----------------
nit: no need for `NOLINT`.


================
Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:17-19
+  auto stringClassMatcher = anyOf(cxxRecordDecl(hasName("string")),
+                                  cxxRecordDecl(hasName("__versa_string")),
+                                  cxxRecordDecl(hasName("basic_string")));
----------------
aaron.ballman wrote:
> These should be using elaborated type specifiers to ensure we get the correct class, not some class that happens to have the same name. Also, this list should be configurable so that users can add their own entries to it.
+1, we could add a `StringLikeClasses` option.


================
Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:48
+                             ->getImplicitObjectArgument();
+
+  // Get the source code blocks (as characters) for both the string object
----------------
nit: add `assert` to make sure these pointers are not nullptr.


================
Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:81
+  auto include_hint = include_inserter_->CreateIncludeInsertion(
+      source.getFileID(expr->getLocStart()), "third_party/absl/strings/match.h",
+      false);
----------------
The include path maybe different in different project.

I think we also need an option for the inserting header, and make it default to `absl/strings/match.h`.


================
Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:92
+      compiler.getSourceManager(), compiler.getLangOpts(),
+      clang::tidy::utils::IncludeSorter::IS_Google);
+  compiler.getPreprocessor().addPPCallbacks(
----------------
We need to make the style customizable by adding an `IncludeStyle` option (see for an example http://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-value-param.html#cmdoption-arg-includestyle), instead of hard-coding.


================
Comment at: docs/clang-tidy/checks/absl-string-find-startswith.rst:17
+should be replaced with
+``if (absl::StartsWith(s, "Hello World")) { /* do something */ };``
+
----------------
Eugene.Zelenko wrote:
> Please use .. code-block: c++
nit: I would keep the `string s = "..."` here.


================
Comment at: test/clang-tidy/absl-string-find-startswith.cpp:1
+// RUN: %check_clang_tidy %s absl-string-find-startswith %t -- -- -I%S
+// -std=c++11
----------------
Since your test is isolated, `// RUN: %check_clang_tidy %s absl-string-find-startswith %t` should work.


================
Comment at: test/clang-tidy/absl-string-find-startswith.cpp:27
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use absl::StartsWith instead of
+  // find() == 0 [absl-string-find-startswith] CHECK_FIXES:
+  // {{^}}absl::StartsWith(s, "a");{{$}}
----------------
nit: we usually use `CHECK-FIXES` in a new line: `// CHECK-FIXES: absl::StartsWith(s, "a");`

The same below.


================
Comment at: test/clang-tidy/absl-string-find-startswith.cpp:30
+
+  s.find(s) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use absl::StartsWith instead of
----------------
Could you also add a test like `if (s.find(s) == 0) { ... }` to make sure the replacement range is correct.

Also maybe add `0 == s.find(s)`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847