- User Since
- Mar 14 2013, 3:16 PM (218 w, 4 d)
Sat, May 20
As an FYI, there is a related check being implemented in clang currently; we probably should not duplicate this effort. See https://reviews.llvm.org/D33333.
As an FYI, there is a related check currently under development in clang-tidy; we probably should not duplicate this functionality in both places. See https://reviews.llvm.org/D19201 for the other review.
Thu, May 18
Fri, May 12
Thu, May 11
Please be sure to regenerate the AST matcher documentation as well by running clang/docs/tools/dump_ast_matchers.py
Wed, May 10
This looks reasonable to me.
Tue, May 9
Have you tried running this over a large code base to see how frequently the diagnostic triggers? I'm wondering why 10 was chosen as the default threshold.
Mon, May 8
Committed in r302419, thank you!
LGTM, thank you! Do you need me to commit this for you?
Fri, May 5
Thanks! I've commit in r302275.
Wed, May 3
Thank you for working on this check!
Mon, May 1
There appears to be a lot of unrelated changes in this patch. For instance Fuchsia-stage2.cmake, NamespaceEndCommentsFixerTest.cpp, and things in CommentSema.h.
I'm adding Richard to the review because he may have opinions on this functionality.
The attribute is not used anywhere; the initial utility should be part of the patch introducing the attribute, unless that utility makes the patch very large.
I've commit in r301796. Rather than accept my own review and then close it, I am marking it as abandoned.
Fri, Apr 28
Wed, Apr 26
Mon, Apr 24
I've commit in r301185, thank you!
Sun, Apr 23
This continues to LGTM; do you need someone to land the patch for you?
Ping. (If I don't hear back, I will assume this is noncontroversial and go ahead and commit sometime next week.)
Apr 19 2017
I'm surprised this change didn't cause any other tests to need to be updated. A few small formatting nits and a request for another test, but otherwise looking good.
Apr 18 2017
Apr 15 2017
... which has the wrong precedence; an extra set of parens is necessary.
However, I don't want to add a set of parens if it isn't necessary. ...
Thanks! I've commit in r300400.
Apr 14 2017
Apr 13 2017
This seems reasonable to me, but you should wait for confirmation before committing (I'm not as familiar with the mangler as others are).
Apr 11 2017
Commit in r299981.
Addressing review comments.
Addressing David's review feedback.
Apr 10 2017
There are a few small nits I've mentioned, but otherwise LGTM.
There appears to be two reviews out for this same functionality. You should probably close one of the reviews (but still address the comments from it).
Please remove the svn prop changes for the two test files. Also, I'd like to see a test that this is properly diagnosed as both a function and a type attribute on a non-x86 architecture, as well as tests that it is properly diagnosed on a type other than a function pointer as well as with arguments on a function pointer (the current tests only test on a declaration rather than a type).
LGTM; I don't think this needs a test case.
Apr 7 2017
This LGTM, but you should give @rsmith some time to review before committing in case he catches something I've missed.
Apr 5 2017
Apr 4 2017
Adding Richard, since this is a fairly extensive change to the frontend and he may have some opinions as well.
Thank you for the update! I think this is getting close, though I have a few more comments inline.
LGTM. but you should wait for someone more familiar with this part of codegen before committing.