hokein added inline comments.
================
Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:1
+#include "StringFindStartswithCheck.h"
+
----------------
nit: We need a LICENSE comment at the top of the file.
================
Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:7
+
+using namespace clang::ast_matchers; // NOLINT: Too many names.
+
----------------
nit: no need for `NOLINT`.
================
Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:17-19
+ auto stringClassMatcher = anyOf(cxxRecordDecl(hasName("string")),
+ cxxRecordDecl(hasName("__versa_string")),
+ cxxRecordDecl(hasName("basic_string")));
----------------
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.
================
Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:48
+ ->getImplicitObjectArgument();
+
+ // Get the source code blocks (as characters) for both the string object
----------------
nit: add `assert` to make sure these pointers are not nullptr.
================
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);
----------------
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`.
================
Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:92
+ compiler.getSourceManager(), compiler.getLangOpts(),
+ clang::tidy::utils::IncludeSorter::IS_Google);
+ compiler.getPreprocessor().addPPCallbacks(
----------------
We need to make the style customizable by adding an `IncludeStyle` option (see
for an example
http://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-value-param.html#cmdoption-arg-includestyle),
instead of hard-coding.
================
Comment at: docs/clang-tidy/checks/absl-string-find-startswith.rst:17
+should be replaced with
+``if (absl::StartsWith(s, "Hello World")) { /* do something */ };``
+
----------------
Eugene.Zelenko wrote:
> Please use .. code-block: c++
nit: I would keep the `string s = "..."` here.
================
Comment at: test/clang-tidy/absl-string-find-startswith.cpp:1
+// RUN: %check_clang_tidy %s absl-string-find-startswith %t -- -- -I%S
+// -std=c++11
----------------
Since your test is isolated, `// RUN: %check_clang_tidy %s
absl-string-find-startswith %t` should work.
================
Comment at: test/clang-tidy/absl-string-find-startswith.cpp:27
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use absl::StartsWith instead of
+ // find() == 0 [absl-string-find-startswith] CHECK_FIXES:
+ // {{^}}absl::StartsWith(s, "a");{{$}}
----------------
nit: we usually use `CHECK-FIXES` in a new line: `// CHECK-FIXES:
absl::StartsWith(s, "a");`
The same below.
================
Comment at: test/clang-tidy/absl-string-find-startswith.cpp:30
+
+ s.find(s) == 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use absl::StartsWith instead of
----------------
Could you also add a test like `if (s.find(s) == 0) { ... }` to make sure the
replacement range is correct.
Also maybe add `0 == s.find(s)`?
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
