User Details
- User Since
- Mar 14 2013, 3:16 PM (421 w, 4 d)
Yesterday
FWIW, it looks like tests are still failing on Windows according to the CI pipeline.
LGTM, thanks!
Fri, Apr 9
Thank you for this patch, I think it's really useful functionality for plugin authors!
Mostly just nits from me, but the attribute portions look good to me.
This continues to LGTM, so I'm accepting it. If @alexfh has any remaining concerns, hopefully he can raise them quickly or we can handle them post-commit. Thanks!
LGTM!
Thu, Apr 8
Thank you for the patch! Btw, can you add more context to the patch when you generate it (I usually use -U 999 when making patches)?
Wed, Apr 7
Tue, Apr 6
LGTM, thank you!
I think the behavior is correct, but I do think the original diagnostic was somewhat better -- an attribute list *can* appear there, if you're lucky. Of course, the old diagnostic isn't great either because adding a ; after the attribute isn't going to improve the situation for the user. So I think I'm actually fine either way, but maybe @rsmith has a preference.
Mon, Apr 5
I've commit in 9711118d2edf7aed133616de1eb7f633c263c4b5, thanks for the really quick reviews!
Adding a few more reviewers for early feedback.
Sun, Apr 4
Sat, Apr 3
Thank you for the additional test case, this LGTM again.
Fri, Apr 2
Committed in 4be8a26951da9a6e04de327b38dd158f4c8e3280
Accepting on my own authority so I can close in a moment.
Correcting lint warnings, adding documentation for the new feature.
@rsmith -- I am posting the review so that I can see how the CI pipeline likes the functionality (because this impacts so many different attributes), but I intend to commit it on my own authority once CI passes. Feel free to review it if you'd like and I'll address any concerns you have post-commit.
Thu, Apr 1
The CI is showing build failures and there are some clang-tidy nits to be addressed as well.
LGTM!
Thank you for picking this back up!
Wed, Mar 31
LGTM, thank you for the fixes!
LGTM, thank you for the patch!
Generally LGTM but I did have a question about the feature testing macro.
Tue, Mar 30
Thanks for merging, I think this seems reasonable but is missing some test coverage still.
LGTM aside from the test case commenting nit. Thank you!
Mon, Mar 29
LGTM with a minor improvement for attribute ordering.
Mostly looks good, just a few things with the tests.
To be clear, this is expected to be an NFC change that allows D97362 to be applied without breaking bots? If so, it'd be helpful to have the changes as part of D97362 because they're critical to that review and so that we get appropriate CI pre-merge testing.
To be clear, this is expected to be an NFC change that allows D97362 to be applied without breaking bots? If so, it'd be helpful to have the changes as part of D97362 because they're critical to that review and so that we get appropriate CI pre-merge testing.
Mostly grammar related review comments, nothing substantive.
Thanks for working on this! I think the direction is good in general, but I think we should also add tests for use in the preprocessor (#if 1z == 1, etc) as well as tests for the behavior in C.
Sun, Mar 28
Fri, Mar 26
Thu, Mar 25
Do you need me to commit on your behalf? If so, which email address would you like me to use for attribution?
LGTM, thanks!
Wed, Mar 24
LGTM, thanks!
I'd really like to hear from @delesley about these changes, specifically because of this bit:
LGTM, thanks!
LGTM, thank you!
I think this basically LGTM with a few minor nits, but I'd like to make sure @rsmith doesn't have concerns, so please wait a few days before landing in case he wants to chime in.
Tue, Mar 23
LGTM!
LGTM!
LGTM!
Mon, Mar 22
Thanks! I've commit on your behalf in 5a87f81fe9aee996dfe3a84dd833f0a48e093e7f
Sun, Mar 21
LGTM, thank you!