- User Since
- Mar 14 2013, 3:16 PM (358 w, 5 d)
Committed in 554791928088d6139e0fb3480d79cd76ea59198f
LGTM! Do you need someone to commit on your behalf?
Adding some more Windows reviewers to see if there are more opinions on how to handle this.
LGTM aside from some minor nits.
You should also regenerate the AST matcher documentation by running clang\docs\tools\dump_ast_matchers.py
Sat, Jan 25
Thank you for the work on this!
LGTM aside from some minor nits.
LGTM, but I'll echo what @rjmccall said about this level of granularity being a bit too fine for review for future patches (we expect changes to be testable, which this is, but only if you also review other patches and notice the tests and how they relate to this patch, which can be easy for reviewers to miss).
Fri, Jan 24
Continues to LG, do you need me to commit on your behalf?
There should also be a mention of this in the release notes (especially if the default behavior winds up changing).
This looks like it's an NFC patch where the only change is to hoist this functionality out into its own interface. Is that correct? If so, can you please be sure to add NFC to the commit log?
Thu, Jan 23
Wed, Jan 22
I've commit on your behalf in ecc7dae50c41bc8a129a158ecf0ae0270126505c. Sorry about the delay in committing and thank you for the patch!
Tue, Jan 21
(There are still some minor whitespace nits to resolve as well.)
Mon, Jan 20
LGTM! Do you think this should also be pushed onto the release branch?
Btw, do you need me to commit this on your behalf, or have you obtained your commit privileges?
Sat, Jan 18
LGTM with a minor testing request, thank you!
Fri, Jan 17
Thank you for working on this, I think this is a great improvement. I am not super qualified to review the cmake, but it looks eminently reasonable to me.
Thank you for the new checker -- I've committed on your behalf in 42a0355816d3bc125d59cbd07052c8886e78ca86
Thu, Jan 16
Thank you for the patch, I've commit on your behalf in d5c6b8407c12d39a78f42a216369407cb2d7b511
LGTM with a few nits.
Given that the compiler already has -Wsign-conversion, I'm not certain what value is added by this check. Can you explain a bit about why this is the correct approach for diagnosing the issue? When you ignore the false positives from the test, the only cases not pointed out by -Wsign-compare are the macro cases (which is reasonable behavior to not diagnose on -- the fix for one macro may make other macros invalid).
Wed, Jan 15
Can you give some examples of code where you think the diagnostics are inappropriate? One of the goals of role-based capabilities is for you to name the capabilities as you see fit, so it's possible that there is a different way for us to surface those diagnostics for your situation that doesn't involve adding another one-off name for capabilities.
Aside from changing the attribute name back to what it was (sorry about that), this LGTM!
Thank you for the patch, I've committed on your behalf in ee0f1f1edc3ec0d4e698d50cc3180217448802b7
Tue, Jan 14
Thank you for the new check, I've commit on your behalf in 36fcbb838c8f293f46bfed78c6ed8c177f1e3485
The changes LGTM, but I notice that our DR status page says we already supported CWG2292 as of Clang 9 (Richard changed the status in svn:368941) which surprises me. Was this aspect just missed with the original commit, or is something else going on?
I think this patch is getting pretty close to finished -- have you tried running it over a large corpus of code to see if it spots reserved identifiers (or unreserved ones)? If not, I would recommend running it over LLVM + clang + clang-tools-extra to see how it operates when looking for reserved identifiers and libc++ for unreserved ones.
Do you need someone to commit this on your behalf? (If you plan to continue contributing, which I hope you do, it may be a good time for you to obtain commit privileges: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access but I am happy to commit on your behalf if you'd prefer.)