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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I need a bit more context because I'm unfamiliar with `absl`. What is this module's intended use?



================
Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:17-19
+  auto stringClassMatcher = anyOf(cxxRecordDecl(hasName("string")),
+                                  cxxRecordDecl(hasName("__versa_string")),
+                                  cxxRecordDecl(hasName("basic_string")));
----------------
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.


================
Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:44-47
+  const auto *expr = result.Nodes.getNodeAs<BinaryOperator>("expr");
+  const auto *needle = result.Nodes.getNodeAs<Expr>("needle");
+  const auto *haystack = result.Nodes.getNodeAs<CXXMemberCallExpr>("findexpr")
+                             ->getImplicitObjectArgument();
----------------
Btw, these can use `auto` because the type is spelled out in the initialization.


================
Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:69
+  auto diag_out = diag(expr->getLocStart(),
+                       (StringRef("Use ") + startswith_str +
+                        " instead of find() " + expr->getOpcodeStr() + " 0")
----------------
Diagnostics shouldn't start with a capital letter (or use terminating punctuation).


================
Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:72
+                           .str())
+                  << FixItHint::CreateReplacement(expr->getSourceRange(),
+                                                  (startswith_str + "(" +
----------------
This fixit should be guarded in case it lands in the middle of a macro for some reason.


================
Comment at: clang-tidy/absl/StringFindStartswithCheck.h:25
+  using ClangTidyCheck::ClangTidyCheck;
+  void registerPPCallbacks(CompilerInstance &compiler) override;
+  void registerMatchers(ast_matchers::MatchFinder *finder) override;
----------------
`compiler` doesn't match our naming conventions (same goes for many other identifiers in the check).


================
Comment at: docs/ReleaseNotes.rst:60
 
+- New `absl-string-find-startswith
+  <http://clang.llvm.org/extra/clang-tidy/checks/absl-string-find-startswith.html>`_ check
----------------
The release notes should also document that this is adding an entirely new module to clang-tidy and what that module's purpose is.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847