User Details
- User Since
- Aug 15 2014, 7:40 AM (411 w, 2 h)
Tue, Jun 14
LGTM
Wed, Jun 8
Empty lines handling should also make it skipped, fix the 3 failures.
LGTM, thanks for the detailed explanation/comments. One minor nitpick inline.
Just noticed that 3 test cases failed. Please fix them before landing.
May 26 2022
Hi, nice to see this getting in. Comments inline!
May 25 2022
May 24 2022
Apr 6 2022
Apr 5 2022
Nice!
Mar 30 2022
Forgot to ask, is there anything preventing a testcase to be added here?
Mar 25 2022
LGTM
Mar 21 2022
This is pretty cool, I enjoy the idea of getting a tar out of a crash. I'm also a +1 for having this group of behaviors as a more official -femit-reproducer=<option> flag. In future work, do you plan to change the default crash mode to output a tar instead of multiple files?
Mar 7 2022
Why isn't D23934 sufficient? What do these flags with very long names do that ffixed-date-time doesn't?
Mar 3 2022
It would be nice to have some mechanism to notify developers that includes are still performed regardless of requires
LGTM
Nice cleanup, fixes and testcases, just pending test failures and this looks good.
LGTM
LGTM
Feb 18 2022
Could the test be tagged with keywords that would prevent the linting to kick in? The amount of changes for this and following patches seems too big to be worth the reformatting.
Feb 7 2022
The right solution to the paren problem is to add some notion of precedence (and associativity) to Nodes, but that's a larger change that would become simpler once the refactoring I'm doing is completed.
Feb 1 2022
LGTM
Nov 19 2021
Nov 18 2021
Given context from further patches in the set, I believe ptrauth operand bundle has been a good way to represent this and works well. LGTM
This is good to go, sorry for the delay.
LGTM
Nov 9 2021
Hi @modimo, can you also add a driver test to check (clang/test/Driver/...) that nothing is added to cc1 cmdline when -fno-new-infallible is used or take precendence?
Thanks for working on upstreaming this @ab. Overall looks good to me, I see clang-format issues, are those legit? One more comment inline.
Nov 8 2021
Seems like the build failures are unrelated, LGTM with the suggested clang-format changes.
Oct 25 2021
Nice catch, thanks for working on this!
Oct 12 2021
Overall looks great but there are some test failures, looks like you also need to update line 223?
Jun 22 2021
Jun 21 2021
Ping
Jun 18 2021
Jun 17 2021
@dexonsmith and @jansvoboda11 thanks for the fast reply and the extra testing.
Jun 15 2021
Assuming I'm answering the correct question that the returns_nonnull is preserved through a PCH, the answer is yes.
Thanks for working on this, comments inline. @vsapsai @jansvoboda11 @dexonsmith any headermap related concerns on your side?
Ping
Jun 7 2021
Ping
Jun 4 2021
Thanks for adding more tests! LGTM.
Jun 2 2021
LGTM!
Jun 1 2021
LGTM
Ping.
@yln sorry, I didn't get notifications for this somehow. This got fixed as part of the more general support in https://github.com/llvm/llvm-project/commit/819e0d105e84c6081cfcfa0e38fd257b6124553a
May 27 2021
Overall looks good, few remaining nitpicks.
LGTM
May 26 2021
May 25 2021
May 24 2021
May 21 2021
LGTM. An additional review here would be nice though.
May 20 2021
Thanks for adding the tests. LGTM after some remaining nitpick.
Update after last round of reviews.
Nothing else of substance from me, but I don't feel confident in giving final sign off on this. LG, but please wait until one of the other reviewers accepts as well.
Overall looks good, sounds like it's failing tests though?
LGTM
Sounds reasonable to me! Can you double check whether this attribute gets correctly serialized/deserialized in face of CXXNewExpr? An example of how to test that would be in clang/test/PCH/cxx-method.cpp.
May 19 2021
May 18 2021
Update patch after @aaron.ballman review!
Thanks for the review @aaron.ballman!
May 17 2021
Make this work for c++14 as well, update testcase to cover all possibilities mentioned. @mdreseler thanks for bringing this up.
May 14 2021
Looks good to me with the remaining clang-format linting on the assert.
May 13 2021
Overall looks good, one more comment below.
You should also update the title with [NFC] or similar.
The approach looks good. There are some test failures - you need to update some bits in clangd and probably anywhere else in the repo that you see uses of getUsingDecl().
May 12 2021
Update patch to reduce code dup as suggested by @dvyukov
May 11 2021
Cool, thanks for the input!
May 10 2021
Thank you @dvyukov for another round of reviews! A have more questions for you.
May 6 2021
Cover more cases in atomic.ll.
Apply more reviewer suggested changes: first evaluate CAS, then decide what order to use.
Forgot to add the Differential tag, my bad. Closed by https://reviews.llvm.org/rG819e0d105e84
LGTM, except that the commit message should say "Lift", not "Lyft". ;)
Reduce the diff in a test.
Update after @rjmccall comments!