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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:17-19
+  auto stringClassMatcher = anyOf(cxxRecordDecl(hasName("string")),
+                                  cxxRecordDecl(hasName("__versa_string")),
+                                  cxxRecordDecl(hasName("basic_string")));
----------------
niko wrote:
> hokein wrote:
> > 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.
> I've made the list configurable. Can you clarify what I would need to add in terms of type specifiers?
You should be looking for a record decl with the name `::std::string` so that you don't run into issues with things like: `namespace terrible_person { class string; }`


================
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();
----------------
niko wrote:
> aaron.ballman wrote:
> > Btw, these can use `auto` because the type is spelled out in the initialization.
> Thanks for the clarification. Is this true even for `Haystack` (as `->getImplicitObjectArgument()`'s type is Expr)?
It's not true for `haystack`, I think I highlighted one too many lines there. :-)


================
Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:72
+                           .str())
+                  << FixItHint::CreateReplacement(expr->getSourceRange(),
+                                                  (startswith_str + "(" +
----------------
niko wrote:
> aaron.ballman wrote:
> > This fixit should be guarded in case it lands in the middle of a macro for some reason.
> Sorry, could you elaborate?
We generally don't issue fixit hints if the source location for the fixit is in a macro. You can test that with `expr->getLocStart().isMacroID()`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847