User Details
- User Since
- Jun 28 2016, 8:37 AM (351 w, 5 d)
Fri, Mar 24
While I'd definitely like to see the pre-commit bots finish before committing this, Aaron tells me there seems to be a series of problems (we had an outage earlier this week, we might be seeing more from it!). Feel free to commit without waiting, post-commit works, so they can catch anything that went wrong.
LGTM! Let me know if you need someone to commit this for you, and include "Name To Use <EmailAddr>"
Test looks fine, still need a release note.
Thu, Mar 23
Just noticed your commit-notice reference to the 'breaking' change is incorrect, should be:
https://reviews.llvm.org/D139705
This needs a test and a release note. Patch otherwise looks fine to me.
This generally looks good to me, but I get lost in the parser pretty quick, so hoping one of the other reviewers can take a look.
Tue, Mar 21
Mon, Mar 20
Changes aaron suggested. Chose zip_longest instead of enumerate.
I've got the 1 concern with the mangling that I REALLY want one of our codegen owners to chime in on, otherwise this LGTM.
Added the code owners here. There doesn't seem to be anything questionable here to me, but I want to make sure someone who really knows what is going on takes a look.
Fri, Mar 17
Still LGTM.
To highlight the fix. See the rest of our release notes.
Please add a release note as requested.
Thu, Mar 16
Please add the test cases requested, plus a Release Note. Otherwise LGTM.
Wed, Mar 15
Still LGTM!
Ah, also, before commit, ensure that pre-commit CI passes.
So I'm not a great person to review this, and I don't have a good idea what this is doing. I'm hopeful that @craig.topper can.
Thank you! This looks good to me. I presume you don't have commit access, so if you give me your "SomeName <EmailAddress>", I can commit this for you.
Tue, Mar 14
Looks good! Thanks! Commit when you're ready.
LGTM with 2 nits.
Thanks for your patience, I was buried trying to get the 16.0 release done. This LGTM!
Mon, Mar 13
First, we've been dealing with the 16.0 release process/regressions, which unfortunately got us pretty far behind on reviews. Sorry about that!
This needs a release note. Also, the patch message doesn't do a good job explaining what is going on here.
Patch seems to be missing all the context.
sorry for the delay, just started unburying myself from the 16.0 release process. Its unfortunate we don't have a great way to test this, but I can see the value of having it anyway in clangd.
Generally looks good to me. Do we do anything special if there are multiple initializers? Also, can we have a codegen test that validates that we actually construct it correctly (and perhaps a constexpr test for the same!)?
Thanks for the ping. I don't have any better ideas/other thoughts, so LGTM.
Patch topic needs "WIP" out of it I think?
Fri, Mar 10
So here's a potential idea for future development: It isn't uncommon/untypical for an attribute to want to return something other than '1', for 'version' (usually an integral value representing a date). The standard attributes all do this. It might be worth looking into some infrastructure to do that.
I want to wait for @aaron.ballman to answer, but i think this is generally OK. Note the motivation is the ability to check this from has_cpp_attribute/has_c_attribute/etc.
Looks like this doesn't compile pre-commit, though no idea if that is a patch-stack issue. Other than test, patch looks fine.
Looks like you have a clang-format fix to do (see pre-commit CI), else, LGTM.
Thu, Mar 9
clang-format fixes, going to make sure this passes CI, then commit
Wed, Mar 8
Add tests from regression bug report.
Going to add 1 more set of test beyond the revert, which is the tests from that regression report.
Tue, Mar 7
Patch itself seems fine enough, but I want to give others a chance to poke at it. The name makes me grumbly, but if there is sufficient 'prior art' here, I'm ok with it.
Other than the two auto's missing a *, I think this is alright. I want to think about it/give others a chance to review, so please ping this in a few days if it hasn't been accepted by then.
Mon, Mar 6
Not thrilled about 'mangled name' still, but I can't think of anything better after all this time, so LGTM.
Tue, Feb 28
This needs a release note. Otherwise I just have a preference for the 'is this spelling/syntax combo provided by this attribute' being in the plugin infrastructure. It DOES make me wonder however, what happens if TWO plugins provide a different 'true' answer for those?