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

[Niko Weh via Phabricator via cfe-commits]

niko added a comment.

Thanks everyone for your comments! I renamed the namespace and filenames to 'abseil'.

@Eugene.Zelenko, definitely interested in extending this to a C++20 modernize check and adding `absl::EndsWith()` support,  would it be OK though to do this in a later patch?



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


================
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();
----------------
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)?


================
Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:72
+                           .str())
+                  << FixItHint::CreateReplacement(expr->getSourceRange(),
+                                                  (startswith_str + "(" +
----------------
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?


================
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);
----------------
hokein wrote:
> 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`.
Instead of having a "AbseilStringsMatchHeader" option, does it make sense to have a "AbseilIncludeDir" option here & default that to `absl`? (esp. if there are going to be other abseil-checks in the future...)


================
Comment at: docs/clang-tidy/checks/absl-string-find-startswith.rst:18
+``if (absl::StartsWith(s, "Hello World")) { /* do something */ };``
+
----------------
Eugene.Zelenko wrote:
> Is there any online documentation about such usage? If so please refer to in at. See other guidelines as example.
I don't believe there is documentation for this yet. The [comment in the header](https://github.com/abseil/abseil-cpp/blob/9850abf25d8efcdc1ff752f1eeef13b640c4ead4/absl/strings/match.h#L50) explains what it does but doesn't have an explicit usage example. (If that qualifies I'll obviously include it?)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847