- User Since
- Mar 14 2013, 3:16 PM (480 w, 1 d)
Wed, May 25
I think this has sat for long enough and my LGTM will have to suffice. :-)
Please add more details to the summary and remove the rdar link (nobody outside of Apple can access that anyway). Also, this should have a release note for the bug fix.
LGTM (minor nit with release notes, take it or leave it at your discretion).
Adding the language WG as a reviewer in case others have opinions.
Update based on review feedback.
Thank you for the ping, this fell entirely off my radar!
The changes so far look sensible, but I think we should add some more tests for a few situations. 1) Using a const weak symbol as a valid initializer should be diagnosed (with a warning? with an error?) so users are alerted to the behavioral quirks. 2) Using a const weak symbol in a constant expression context should probably be an error, right? e.g.,
const int hmm __attribute__((weak)) = 12; int erp = hmm; // Error in C, dynamic init in C++?
Tue, May 24
Thank you for your work on this, I generally like this new approach and think we're heading in the right direction.
LGTM -- I checked as well and these changes are NFC. Thanks!
LGTM, though this should have a release note for the fix.
LGTM, thank you for catching this!
LGTM aside from a testing request and a nit. Thanks for the fix!
Mon, May 23
LGTM, thank you!
Thanks for this! It generally LGTM (though my Python skills are not particularly awesome, so take it with a grain of salt). Just a few questions at this point.
Sun, May 22
Sat, May 21
Fri, May 20
The primary application is for more rapid debugging of the amdgpu backend by permuting a C or C++ test file instead of manually updating an IR file.
Thu, May 19
LGTM, thank you!
Fixes failed AST matching unit test and speculatively fixes a CodeGen test, all found via precommit CI testing.
Thank you for this! Is there a way to add test coverage for this change? Also, this should come with a release note as it's fixing a bug someone reported.
Wed, May 18
Adding some Apple reviewers because this touches ObjC behavior (I would be very surprised if ObjC wanted different rules here than C though).
Went with a more simple implementation approach and the diagnostic results seem to be a mild improvement in terms of wording and behavior.
Tue, May 17
Btw, I'm in C standards meetings all week this week, so my reviews are going to be delayed by a bit, just as an FYI.
I know it's retroactive, but this also LGTM. Thank you for the fix!
Mon, May 16
It's not clear to me why existing facilities shouldn't be extended to cover this case rather than coming up with another feature testing macro. There's already plenty of confusion for users to decide between __has_feature and __has_extension, and now we're talking about adding a third choice to the mix.
LGTM, thank you for this!
Note, I'm in C standards committee meetings all week this week, so I expect I won't be doing many reviews this week and I'll be catching up as best I can starting next week.