[PATCH] D80023: [clang-tidy] Add abseil-string-find-str-contains checker.
tdl-g added a comment.
Thanks, all, for the additional comments. I addressed them all except for the
suggestion to add an options-specific test. I'm not against it, but (as I
mention in the comment) I'm also unsure how to meaningfully test the
include-inserting-related options.
Comment at:
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp:72
+ binaryOperator(hasOperatorName("=="),
+ hasEitherOperand(ignoringParenImpCasts(StringNpos)),
+ hasEitherOperand(ignoringParenImpCasts(StringFind))),
njames93 wrote:
> ymandel wrote:
> > Would `hasOperands` replace these two separate calls to `hasEitherOperand`?
> > Same below lines 79-80 (I think it's a new matcher, fwiw).
> Just added, I added it to remove calls of multiple hasEitherOperand calls for
> the reason of preventing the matchers matching the same operand. I just got a
> little lost in my previous comment
Switched to hasOperands. I agree that expresses the intent better than two
independent calls of hasEitherOperand.
Note that it was working fine before--the test cases confirmed that it wasn't
matching string::npos == string::npos or s.find(a) == s.find(a). I'm guessing
that's because the matchers inside hasEitherOperand have bindings, and if the
matcher matched but one of the bindings was missing, the rewriterule suppressed
itself? Is this a plausible theory?
(The question moot since hasOperands is better overall, but I'd like to
understand why it was working before.)
Comment at:
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp:98
+const LangOptions &LangOpts) const {
+ return LangOpts.CPlusPlus;
+}
njames93 wrote:
> nit: as abseil requires c++11, should this check also require c++11 support
Done, though I note that none of the other checkers in the abseil directory are
checking for CplusCplus11. (That's not an argument against doing it, just a
question of whether or not they should also get this change at some point.)
Comment at:
clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp:2
+// RUN: %check_clang_tidy %s abseil-string-find-str-contains %t -- \
+// RUN: -config="{CheckOptions: []}"
+
ymandel wrote:
> I'm not sure what's standard practice, but consider whether its worth adding
> another test file that tests the check options. It wouldn't have to be
> comprehensive like this one, just some basic tests that the options are being
> honored.
I'm open to it, but note that two of the three options (IncludeStyle, inherited
from TransformerClangTidy) and AbseilStringsMatchHeader, only manifest in the
header-inclusion.
So if I had a separate test it would be easy to confirm that StringLikeClasses
was being honored. But I'm not sure how I'd confirm that IncludeStyle or
AbseilStringsMatchHeader are being honored. Suggestions?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80023/new/
https://reviews.llvm.org/D80023
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D80023: [clang-tidy] Add abseil-string-find-str-contains checker.
tdl-g updated this revision to Diff 265234.
tdl-g marked 12 inline comments as done.
tdl-g added a comment.
Addressed second round of comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80023/new/
https://reviews.llvm.org/D80023
Files:
clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp
clang-tools-extra/clang-tidy/abseil/CMakeLists.txt
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/abseil-string-find-str-contains.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
@@ -0,0 +1,290 @@
+// RUN: %check_clang_tidy %s abseil-string-find-str-contains %t -- \
+// RUN: -config="{CheckOptions: []}"
+
+using size_t = decltype(sizeof(int));
+
+namespace std {
+
+// Lightweight standin for std::string.
+template
+class basic_string {
+public:
+ basic_string();
+ basic_string(const basic_string &);
+ basic_string(const C *);
+ ~basic_string();
+ int find(basic_string s, int pos = 0);
+ int find(const C *s, int pos = 0);
+ int find(char c, int pos = 0);
+ static constexpr size_t npos = -1;
+};
+typedef basic_string string;
+
+// Lightweight standin for std::string_view.
+template
+class basic_string_view {
+public:
+ basic_string_view();
+ basic_string_view(const basic_string_view &);
+ basic_string_view(const C *);
+ ~basic_string_view();
+ int find(basic_string_view s, int pos = 0);
+ int find(const C *s, int pos = 0);
+ int find(char c, int pos = 0);
+ static constexpr size_t npos = -1;
+};
+typedef basic_string_view string_view;
+
+} // namespace std
+
+namespace absl {
+
+// Lightweight standin for absl::string_view.
+class string_view {
+public:
+ string_view();
+ string_view(const string_view &);
+ string_view(const char *);
+ ~string_view();
+ int find(string_view s, int pos = 0);
+ int find(const char *s, int pos = 0);
+ int find(char c, int pos = 0);
+ static constexpr size_t npos = -1;
+};
+
+} // namespace absl
+
+// Functions that take and return our various string-like types.
+std::string foo_ss(std::string);
+std::string_view foo_ssv(std::string_view);
+absl::string_view foo_asv(absl::string_view);
+std::string bar_ss();
+std::string_view bar_ssv();
+absl::string_view bar_asv();
+
+// Confirms that find==npos and find!=npos work for each supported type, when
+// npos comes from the correct type.
+void basic_tests() {
+ std::string ss;
+ ss.find("a") == std::string::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StrContains instead of find() == npos
+ // CHECK-FIXES: {{^[[:space:]]*}}!absl::StrContains(ss, "a");{{$}}
+
+ ss.find("a") != std::string::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of find() != npos
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(ss, "a");{{$}}
+
+ std::string::npos != ss.find("a");
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(ss, "a");{{$}}
+
+ std::string_view ssv;
+ ssv.find("a") == std::string_view::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}!absl::StrContains(ssv, "a");{{$}}
+
+ ssv.find("a") != std::string_view::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(ssv, "a");{{$}}
+
+ std::string_view::npos != ssv.find("a");
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(ssv, "a");{{$}}
+
+ absl::string_view asv;
+ asv.find("a") == absl::string_view::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}!absl::StrContains(asv, "a");{{$}}
+
+ asv.find("a") != absl::string_view::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(asv, "a");{{$}}
+
+ absl::string_view::npos != asv.find("a");
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(asv, "a");{{$}}
+}
+
+// Confirms that it works even if you mix-and-match the type for find and for
+// npos. (One of the reasons for this checker is to clean up cases that
+// accidentally mix-and-match like this. absl::StrContains is less
+// error-prone.)
+void mismatched_npos() {
+ std::string ss
[PATCH] D80023: [clang-tidy] Add abseil-string-find-str-contains checker.
tdl-g updated this revision to Diff 266207.
tdl-g added a comment.
Fixed length of visual separator.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80023/new/
https://reviews.llvm.org/D80023
Files:
clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp
clang-tools-extra/clang-tidy/abseil/CMakeLists.txt
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/abseil-string-find-str-contains.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
@@ -0,0 +1,290 @@
+// RUN: %check_clang_tidy %s abseil-string-find-str-contains %t -- \
+// RUN: -config="{CheckOptions: []}"
+
+using size_t = decltype(sizeof(int));
+
+namespace std {
+
+// Lightweight standin for std::string.
+template
+class basic_string {
+public:
+ basic_string();
+ basic_string(const basic_string &);
+ basic_string(const C *);
+ ~basic_string();
+ int find(basic_string s, int pos = 0);
+ int find(const C *s, int pos = 0);
+ int find(char c, int pos = 0);
+ static constexpr size_t npos = -1;
+};
+typedef basic_string string;
+
+// Lightweight standin for std::string_view.
+template
+class basic_string_view {
+public:
+ basic_string_view();
+ basic_string_view(const basic_string_view &);
+ basic_string_view(const C *);
+ ~basic_string_view();
+ int find(basic_string_view s, int pos = 0);
+ int find(const C *s, int pos = 0);
+ int find(char c, int pos = 0);
+ static constexpr size_t npos = -1;
+};
+typedef basic_string_view string_view;
+
+} // namespace std
+
+namespace absl {
+
+// Lightweight standin for absl::string_view.
+class string_view {
+public:
+ string_view();
+ string_view(const string_view &);
+ string_view(const char *);
+ ~string_view();
+ int find(string_view s, int pos = 0);
+ int find(const char *s, int pos = 0);
+ int find(char c, int pos = 0);
+ static constexpr size_t npos = -1;
+};
+
+} // namespace absl
+
+// Functions that take and return our various string-like types.
+std::string foo_ss(std::string);
+std::string_view foo_ssv(std::string_view);
+absl::string_view foo_asv(absl::string_view);
+std::string bar_ss();
+std::string_view bar_ssv();
+absl::string_view bar_asv();
+
+// Confirms that find==npos and find!=npos work for each supported type, when
+// npos comes from the correct type.
+void basic_tests() {
+ std::string ss;
+ ss.find("a") == std::string::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StrContains instead of find() == npos
+ // CHECK-FIXES: {{^[[:space:]]*}}!absl::StrContains(ss, "a");{{$}}
+
+ ss.find("a") != std::string::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of find() != npos
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(ss, "a");{{$}}
+
+ std::string::npos != ss.find("a");
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(ss, "a");{{$}}
+
+ std::string_view ssv;
+ ssv.find("a") == std::string_view::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}!absl::StrContains(ssv, "a");{{$}}
+
+ ssv.find("a") != std::string_view::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(ssv, "a");{{$}}
+
+ std::string_view::npos != ssv.find("a");
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(ssv, "a");{{$}}
+
+ absl::string_view asv;
+ asv.find("a") == absl::string_view::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}!absl::StrContains(asv, "a");{{$}}
+
+ asv.find("a") != absl::string_view::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(asv, "a");{{$}}
+
+ absl::string_view::npos != asv.find("a");
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(asv, "a");{{$}}
+}
+
+// Confirms that it works even if you mix-and-match the type for find and for
+// npos. (One of the reasons for this checker is to clean up cases that
+// accidentally mix-and-match like this. absl::StrContains is less
+// error-prone.)
+void mismatched_npos() {
+ std::string ss;
+ ss.find("a") == std::string_view::npos
[PATCH] D80023: [clang-tidy] Add abseil-string-find-str-contains checker.
tdl-g marked 4 inline comments as done.
tdl-g added a comment.
Thanks again, addressed all comments.
Comment at:
clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp:2
+// RUN: %check_clang_tidy %s abseil-string-find-str-contains %t -- \
+// RUN: -config="{CheckOptions: []}"
+
tdl-g wrote:
> ymandel wrote:
> > I'm not sure what's standard practice, but consider whether its worth
> > adding another test file that tests the check options. It wouldn't have to
> > be comprehensive like this one, just some basic tests that the options are
> > being honored.
> I'm open to it, but note that two of the three options (IncludeStyle,
> inherited from TransformerClangTidy) and AbseilStringsMatchHeader, only
> manifest in the header-inclusion.
>
> So if I had a separate test it would be easy to confirm that
> StringLikeClasses was being honored. But I'm not sure how I'd confirm that
> IncludeStyle or AbseilStringsMatchHeader are being honored. Suggestions?
Discussed with ymandel offline, we'll add more option-specific tests when we
clean up transformer's options handling.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80023/new/
https://reviews.llvm.org/D80023
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D80023: [clang-tidy] Add abseil-string-find-str-contains checker.
tdl-g added a comment. We see this broke the build for shared-lib config http://lab.llvm.org:8011/builders/llvm-avr-linux/builds/1879 Looking now (and I look forward to determining what I should have done to avoid this). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80023/new/ https://reviews.llvm.org/D80023 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82126: [libTooling] Change `selection` stencil to handle some cases of text in macros.
tdl-g added a comment. LGTM. I found the change description confusing, since it talks about the selection() stencil but the code is all about the cat() stencil. I realize (now) that the former is deprecated in favor of the latter. But the change description is still confusing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82126/new/ https://reviews.llvm.org/D82126 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82126: [libTooling] Change Transformer's `cat` to handle some cases of text in macros.
tdl-g added a comment.
Interesting, in all three of those cases, it's reasonable to replace the entire
expression, thus eliminating the macro. None of those "tear" the macro; if we
had a case like
#define FOO(a,b,c,d) ((a).find(b) == std::string::npos ? (c) : (d))
FOO("helo", "x", 5, 6);
I guess in that case we'd want to suppress an edit change, since it would have
to modify the macro to make the change. But I guess all the existing test
cases are actually safe to convert with the macro.
Do you want to remove the existing macro cases and add the "tearing" one above
to confirm that it doesn't propose an edit?
In D82126#2103934 , @ymandel wrote:
> Tests show that this breaks the test for the clang tidy
> `abseil-string-find-str-contains`. Curious if this is a desirable change in
> behavior (in which case I'll update your test file) or the wrong behavior:
>
> https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp#L246-L252
>
> void no_macros() {
> std::string s;
> - COMPARE_MACRO(s.find("a"), std::string::npos);
> - FIND_MACRO(s, "a") == std::string::npos;
> - FIND_COMPARE_MACRO(s, "a", std::string::npos);
> + !absl::StrContains(s, "a");
> + !absl::StrContains(s, "a");
> + !absl::StrContains(s, "a");
>}
>
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82126/new/
https://reviews.llvm.org/D82126
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D80023: [clang-tidy] Add abseil-string-find-str-contains checker.
tdl-g created this revision.
tdl-g added a reviewer: ymandel.
Herald added subscribers: cfe-commits, phosek, Charusso, mgorny.
Herald added a project: clang.
Eugene.Zelenko edited reviewers, added: alexfh, hokein, aaron.ballman,
njames93; removed: ymandel.
Eugene.Zelenko added a project: clang-tools-extra.
Eugene.Zelenko retitled this revision from "Add abseil-string-find-str-contains
checker." to "[clang-tidy] Add abseil-string-find-str-contains checker.".
Herald added a subscriber: xazax.hun.
This adds a checker which suggests replacing string.find(...) == npos with
absl::StrContains.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D80023
Files:
clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp
clang-tools-extra/clang-tidy/abseil/CMakeLists.txt
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/abseil-string-find-str-contains.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
@@ -0,0 +1,283 @@
+// RUN: %check_clang_tidy %s abseil-string-find-str-contains %t -- \
+// RUN: -config="{CheckOptions: []}"
+
+using size_t = decltype(sizeof(int));
+
+namespace std {
+
+// Lightweight standin for std::string.
+template
+class basic_string {
+public:
+ basic_string();
+ basic_string(const basic_string &);
+ basic_string(const C *);
+ ~basic_string();
+ int find(basic_string s, int pos = 0);
+ int find(const C *s, int pos = 0);
+ int find(char c, int pos = 0);
+ static constexpr size_t npos = -1;
+};
+typedef basic_string string;
+
+// Lightweight standin for std::string_view.
+template
+class basic_string_view {
+public:
+ basic_string_view();
+ basic_string_view(const basic_string_view &);
+ basic_string_view(const C *);
+ ~basic_string_view();
+ int find(basic_string_view s, int pos = 0);
+ int find(const C *s, int pos = 0);
+ int find(char c, int pos = 0);
+ static constexpr size_t npos = -1;
+};
+typedef basic_string_view string_view;
+
+} // namespace std
+
+namespace absl {
+
+// Lightweight standin for absl::string_view.
+class string_view {
+public:
+ string_view();
+ string_view(const string_view &);
+ string_view(const char *);
+ ~string_view();
+ int find(string_view s, int pos = 0);
+ int find(const char *s, int pos = 0);
+ int find(char c, int pos = 0);
+ static constexpr size_t npos = -1;
+};
+
+} // namespace absl
+
+// Functions that take and return our various string-like types.
+std::string foo_ss(std::string);
+std::string_view foo_ssv(std::string_view);
+absl::string_view foo_asv(absl::string_view);
+std::string bar_ss();
+std::string_view bar_ssv();
+absl::string_view bar_asv();
+
+// Confirms that find==npos and find!=npos work for each supported type, when
+// npos comes from the correct type.
+void basic_tests() {
+ std::string ss;
+ ss.find("a") == std::string::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StrContains instead of find() == npos
+ // CHECK-FIXES: {{^[[:space:]]*}}!absl::StrContains(ss, "a");{{$}}
+
+ ss.find("a") != std::string::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of find() != npos
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(ss, "a");{{$}}
+
+ std::string::npos != ss.find("a");
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(ss, "a");{{$}}
+
+ std::string_view ssv;
+ ssv.find("a") == std::string_view::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}!absl::StrContains(ssv, "a");{{$}}
+
+ ssv.find("a") != std::string_view::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(ssv, "a");{{$}}
+
+ std::string_view::npos != ssv.find("a");
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(ssv, "a");{{$}}
+
+ absl::string_view asv;
+ asv.find("a") == absl::string_view::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}!absl::StrContains(asv, "a");{{$}}
+
+ asv.find("a") != absl::string_view::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(asv, "a");{{$}}
+
+ absl::string_view::npos != asv.find("a");
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use
[PATCH] D80023: [clang-tidy] Add abseil-string-find-str-contains checker.
tdl-g added a comment. Eugene, thank you for the comments, I'll address them soon. For the moment I'm trying to figure out what's up with the list.rst changes. Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:15 - `abseil-duration-addition `_, "Yes" - `abseil-duration-comparison `_, "Yes" - `abseil-duration-conversion-cast `_, "Yes" - `abseil-duration-division `_, "Yes" - `abseil-duration-factory-float `_, "Yes" - `abseil-duration-factory-scale `_, "Yes" - `abseil-duration-subtraction `_, "Yes" - `abseil-duration-unnecessary-conversion `_, "Yes" - `abseil-faster-strsplit-delimiter `_, "Yes" + `abseil-duration-addition `_, + `abseil-duration-comparison `_, Eugene.Zelenko wrote: > Unrelated and incorrect changes. Agreed, these were generated by "clang-tidy/add_new_check.py abseil string-find-str-contains", still trying to figure out why. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80023/new/ https://reviews.llvm.org/D80023 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D80023: [clang-tidy] Add abseil-string-find-str-contains checker.
tdl-g updated this revision to Diff 264986.
tdl-g marked 16 inline comments as done.
tdl-g added a comment.
Addressed review comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80023/new/
https://reviews.llvm.org/D80023
Files:
clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp
clang-tools-extra/clang-tidy/abseil/CMakeLists.txt
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/abseil-string-find-str-contains.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
@@ -0,0 +1,290 @@
+// RUN: %check_clang_tidy %s abseil-string-find-str-contains %t -- \
+// RUN: -config="{CheckOptions: []}"
+
+using size_t = decltype(sizeof(int));
+
+namespace std {
+
+// Lightweight standin for std::string.
+template
+class basic_string {
+public:
+ basic_string();
+ basic_string(const basic_string &);
+ basic_string(const C *);
+ ~basic_string();
+ int find(basic_string s, int pos = 0);
+ int find(const C *s, int pos = 0);
+ int find(char c, int pos = 0);
+ static constexpr size_t npos = -1;
+};
+typedef basic_string string;
+
+// Lightweight standin for std::string_view.
+template
+class basic_string_view {
+public:
+ basic_string_view();
+ basic_string_view(const basic_string_view &);
+ basic_string_view(const C *);
+ ~basic_string_view();
+ int find(basic_string_view s, int pos = 0);
+ int find(const C *s, int pos = 0);
+ int find(char c, int pos = 0);
+ static constexpr size_t npos = -1;
+};
+typedef basic_string_view string_view;
+
+} // namespace std
+
+namespace absl {
+
+// Lightweight standin for absl::string_view.
+class string_view {
+public:
+ string_view();
+ string_view(const string_view &);
+ string_view(const char *);
+ ~string_view();
+ int find(string_view s, int pos = 0);
+ int find(const char *s, int pos = 0);
+ int find(char c, int pos = 0);
+ static constexpr size_t npos = -1;
+};
+
+} // namespace absl
+
+// Functions that take and return our various string-like types.
+std::string foo_ss(std::string);
+std::string_view foo_ssv(std::string_view);
+absl::string_view foo_asv(absl::string_view);
+std::string bar_ss();
+std::string_view bar_ssv();
+absl::string_view bar_asv();
+
+// Confirms that find==npos and find!=npos work for each supported type, when
+// npos comes from the correct type.
+void basic_tests() {
+ std::string ss;
+ ss.find("a") == std::string::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StrContains instead of find() == npos
+ // CHECK-FIXES: {{^[[:space:]]*}}!absl::StrContains(ss, "a");{{$}}
+
+ ss.find("a") != std::string::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of find() != npos
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(ss, "a");{{$}}
+
+ std::string::npos != ss.find("a");
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(ss, "a");{{$}}
+
+ std::string_view ssv;
+ ssv.find("a") == std::string_view::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}!absl::StrContains(ssv, "a");{{$}}
+
+ ssv.find("a") != std::string_view::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(ssv, "a");{{$}}
+
+ std::string_view::npos != ssv.find("a");
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(ssv, "a");{{$}}
+
+ absl::string_view asv;
+ asv.find("a") == absl::string_view::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}!absl::StrContains(asv, "a");{{$}}
+
+ asv.find("a") != absl::string_view::npos;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(asv, "a");{{$}}
+
+ absl::string_view::npos != asv.find("a");
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StrContains instead of
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StrContains(asv, "a");{{$}}
+}
+
+// Confirms that it works even if you mix-and-match the type for find and for
+// npos. (One of the reasons for this checker is to clean up cases that
+// accidentally mix-and-match like this. absl::StrContains is less
+// error-prone.)
+void mismatched_npos() {
+ std::string ss;
+ ss.f
[PATCH] D80023: [clang-tidy] Add abseil-string-find-str-contains checker.
tdl-g added a comment.
Thanks, all for the comments. I believe I've addressed all comments. Note
that TransformerClangTidyCheck interacts awkwardly with StoreOptions; I have a
FIXME to clean that up.
Comment at:
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp:71-72
+ binaryOperator(hasOperatorName("=="),
+ hasEitherOperand(ignoringParenImpCasts(StringNpos)),
+ hasEitherOperand(ignoringParenImpCasts(StringFind))),
+ change(cat("!absl::StrContains(", node("string_being_searched"),
njames93 wrote:
> njames93 wrote:
> > This is dangerous, it will match on `std::string::npos ==
> > std::string::npos` and (more importantly) `strA.find(...) ==
> > strB.find(...)`. See D80054.
> Ignore me, its late.
Just to be safe, I added a test case that confirms that it doesn't match on
those constructs.
Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:15
- `abseil-duration-addition `_, "Yes"
- `abseil-duration-comparison `_, "Yes"
- `abseil-duration-conversion-cast `_,
"Yes"
- `abseil-duration-division `_, "Yes"
- `abseil-duration-factory-float `_, "Yes"
- `abseil-duration-factory-scale `_, "Yes"
- `abseil-duration-subtraction `_, "Yes"
- `abseil-duration-unnecessary-conversion
`_, "Yes"
- `abseil-faster-strsplit-delimiter
`_, "Yes"
+ `abseil-duration-addition `_,
+ `abseil-duration-comparison `_,
njames93 wrote:
> tdl-g wrote:
> > Eugene.Zelenko wrote:
> > > Unrelated and incorrect changes.
> > Agreed, these were generated by "clang-tidy/add_new_check.py abseil
> > string-find-str-contains", still trying to figure out why.
> Did you invoke the python script from the clang-tidy folder or the
> clang-tools-extra folder. The script is sensitive to the working directory it
> was executed from, it must be executed from the clang-tidy folder otherwise
> it fails when trying to detect auto fixes.
Possibly. I re-ran it on a clean cloned repo from the clang-tidy folder and it
still created some unrelated list.rst changes (though fewer than before). So
for now I just added the one line
I want by hand.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80023/new/
https://reviews.llvm.org/D80023
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D89468: [libTooling] Change `after` range-selector to operate only on source ranges
tdl-g requested changes to this revision.
tdl-g added a comment.
This revision now requires changes to proceed.
Just one comment about the tests.
Comment at: clang/unittests/Tooling/RangeSelectorTest.cpp:196
+static void testAfterMacroArg(StringRef Code) {
+ const std::string Ref = "ref";
If this helper function took an "expected" parameter I might consider it
self-explanatory, but as it is, it's extremely non-obvious what it does without
reading the body in detail--which means the tests that use it are not
particularly readable either. (Each test reads like "here's the input data,
make sure something unspecified is true about it.") Specifically, this
function searches for a reference to the variable "y" and ensures that after()
finds the character after it. So at a minimum, document that.
I'm also trying to decide if this helper is too similar to the
implementation--tests should not just restate what the production code does,
they should find some other way to validate the behavior. Do you think this is
sufficiently different from the prod implementation to be meaningful? If so,
that's fine. If not, maybe the helper should just take an expected byte
offset, be renamed to "afterYHasByteOffset", and then each test would be a bit
more readable?
Up to you.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89468/new/
https://reviews.llvm.org/D89468
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D92658: [libTooling] Add `describe` stencil for formatting AST nodes for diagnostics.
tdl-g accepted this revision.
tdl-g added a comment.
This revision is now accepted and ready to land.
Looks great, just one comment.
Comment at: clang/unittests/Tooling/StencilTest.cpp:513
+TEST(StencilToStringTest, DescribeOp) {
+ auto S = describe("Id");
Can you add a comment (or a more detailed test name) explaining what this test
case is validating?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92658/new/
https://reviews.llvm.org/D92658
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D92658: [libTooling] Add `describe` stencil for formatting AST nodes for diagnostics.
tdl-g added inline comments.
Comment at: clang/unittests/Tooling/StencilTest.cpp:513
+TEST(StencilToStringTest, DescribeOp) {
+ auto S = describe("Id");
ymandel wrote:
> tdl-g wrote:
> > Can you add a comment (or a more detailed test name) explaining what this
> > test case is validating?
> It's the same pattern for (nearly) all of the StencilToStringTest cases.
> Might a comment on the fixture be a better place?
Oh, sorry, I thought the "repr" was a stencil combinator, I didn't realize it
was part of the raw string quotation. My bad.
LGTM as is.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92658/new/
https://reviews.llvm.org/D92658
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D107837: abseil-string-find-str-contains should not propose an edit for the three-parameter version of find().
tdl-g created this revision.
tdl-g added a reviewer: ymandel.
tdl-g requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
std::string, std::string_view, and absl::string_view all have a three-parameter
version of find()
which has a "count" (or "n") paremeter limiting the size of the substring to
search. We don't want
to propose changing to absl::StrContains in those cases. This change fixes
that and adds unit tests
to confirm.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D107837
Files:
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
Index:
clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
===
---
clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
+++
clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
@@ -15,6 +15,7 @@
~basic_string();
int find(basic_string s, int pos = 0);
int find(const C *s, int pos = 0);
+ int find(const C *s, int pos, int n);
int find(char c, int pos = 0);
static constexpr size_t npos = -1;
};
@@ -30,6 +31,7 @@
~basic_string_view();
int find(basic_string_view s, int pos = 0);
int find(const C *s, int pos = 0);
+ int find(const C *s, int pos, int n);
int find(char c, int pos = 0);
static constexpr size_t npos = -1;
};
@@ -48,6 +50,7 @@
~string_view();
int find(string_view s, int pos = 0);
int find(const char *s, int pos = 0);
+ int find(const char *s, int pos, int n);
int find(char c, int pos = 0);
static constexpr size_t npos = -1;
};
@@ -263,6 +266,18 @@
asv.find("a", 3) == std::string_view::npos;
}
+// Confirms that it does not match when the count parameter is present.
+void no_count() {
+ std::string ss;
+ ss.find("a", 0, 1) == std::string::npos;
+
+ std::string_view ssv;
+ ssv.find("a", 0, 1) == std::string_view::npos;
+
+ absl::string_view asv;
+ asv.find("a", 0, 1) == std::string_view::npos;
+}
+
// Confirms that it does not match when it's compared to something other than
// npos, even if the value is the same as npos.
void no_non_npos() {
Index: clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
===
--- clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
+++ clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
@@ -53,7 +53,7 @@
to(varDecl(hasName("npos"), hasDeclContext(StringLikeClass;
auto StringFind = cxxMemberCallExpr(
callee(cxxMethodDecl(
- hasName("find"),
+ hasName("find"), parameterCountIs(2),
hasParameter(
0, parmVarDecl(anyOf(hasType(StringType), hasType(CharStarType),
hasType(CharType)),
Index: clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
@@ -15,6 +15,7 @@
~basic_string();
int find(basic_string s, int pos = 0);
int find(const C *s, int pos = 0);
+ int find(const C *s, int pos, int n);
int find(char c, int pos = 0);
static constexpr size_t npos = -1;
};
@@ -30,6 +31,7 @@
~basic_string_view();
int find(basic_string_view s, int pos = 0);
int find(const C *s, int pos = 0);
+ int find(const C *s, int pos, int n);
int find(char c, int pos = 0);
static constexpr size_t npos = -1;
};
@@ -48,6 +50,7 @@
~string_view();
int find(string_view s, int pos = 0);
int find(const char *s, int pos = 0);
+ int find(const char *s, int pos, int n);
int find(char c, int pos = 0);
static constexpr size_t npos = -1;
};
@@ -263,6 +266,18 @@
asv.find("a", 3) == std::string_view::npos;
}
+// Confirms that it does not match when the count parameter is present.
+void no_count() {
+ std::string ss;
+ ss.find("a", 0, 1) == std::string::npos;
+
+ std::string_view ssv;
+ ssv.find("a", 0, 1) == std::string_view::npos;
+
+ absl::string_view asv;
+ asv.find("a", 0, 1) == std::string_view::npos;
+}
+
// Confirms that it does not match when it's compared to something other than
// npos, even if the value is the same as npos.
void no_non_npos() {
Index: clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
===
--- clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
+++ clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
@@ -53,7 +53,7 @@
to(varDecl(hasName("npos"), hasDeclContext(StringLikeClass;
auto StringFind = cxxMemberCallExpr(
c
[PATCH] D101037: [clang-tidy] Change shebang from python to python3
tdl-g accepted this revision. tdl-g added a comment. This revision is now accepted and ready to land. I'll defer to the consensus on https://lists.llvm.org/pipermail/cfe-dev/2021-April/068047.html regarding whether or not there are gotchas for requiring python3,, but assuming tests have confirmed that each of these scripts are python3 compatible, this seems reasonable to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101037/new/ https://reviews.llvm.org/D101037 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D93637: [libTooling] Add support for smart pointers to releveant Transformer `Stencil`s.
tdl-g added inline comments. Comment at: clang/unittests/Tooling/StencilTest.cpp:273 + std::string Snippet = R"cc( +Smart x; +x; You're only testing the "QuacksLike" case. I suspect you should have tests that validate the "KnownSmartPointers". Admittedly, it's a bit redundant since the known smart pointers also QuackLike pointers. Which, I guess, raises the question of why you have the hard-coded list of KnownSmartPointers if they are covered by the QuacksLike behavior and thus can't be meaningfully tested independently. What do you think? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93637/new/ https://reviews.llvm.org/D93637 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D93637: [libTooling] Add support for smart pointers to relevant Transformer `Stencil`s.
tdl-g added a comment. Ah, if it's just an optimization that makes sense. I still think it's worth having a test case to confirm that one of the specially-handled cases works. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93637/new/ https://reviews.llvm.org/D93637 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
