niko added a comment.

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

@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()), 
+      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 
 explains what it does but doesn't have an explicit usage example. (If that 
qualifies I'll obviously include it?)

  rCTE Clang Tools Extra

cfe-commits mailing list

Reply via email to