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