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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
