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.
===
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
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/AbseilTidyMo
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: -c
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
http
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:
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 gu
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-too
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"
- `abse
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-ex
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/StringFindStrContainsChec
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::strin
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");
C
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
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 "
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 confirm
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".
Admitte
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://revie
18 matches
Mail list logo
