- User Since
- Feb 16 2014, 1:10 AM (213 w, 1 d)
Thu, Mar 15
Wed, Mar 7
Just tidying up one line with clang-format. I guess everyone's busy preparing for the Jacksonville standards meeting. Please give this a review when you all have a second -- in addition to the reported bug, https://bugs.llvm.org/show_bug.cgi?id=34897, I've also confirmed that this diff fixes ASAN errors that surfaced in a project at work. Thanks! :)
Tue, Mar 6
Update formatting with clang-format.
Wed, Feb 28
I wasn't sure what the best way to test this would be. The assertion occurs in LLVM, but Clang is responsible for scheduling the passes. If anyone has any suggestions, I'd greatly appreciate them!
Thu, Feb 22
Friendly ping! Let me know if this seems totally wrong to you, @GorNishanov :)
Tue, Feb 20
Mon, Feb 19
Great, thank you both for the reviews!
Thanks for the review! I've updated the diff to use range loops instead.
Feb 16 2018
Feb 15 2018
Thank you for all the reviews, @GorNishanov!
Thanks again, @GorNishanov, for all the reviews!
Thank you, @GorNishanov!
Feb 14 2018
Thanks for the review, @GorNishanov, and for pointing out that part of the standard! I added a reference to the implicit object parameter, as well as some tests for that behavior. And thanks for pointing out the flaw in https://reviews.llvm.org/D41820 -- I'll submit a separate diff to forward the implicit object parameter to promise type constructors.
Thanks for the review, @GorNishanov! I added something similar to your example of a store operand to the test case. Indeed, without the changes you proposed, the existence of the operand caused an invariant to be violated in LLVM. With your suggested changes, the assertion doesn't fire and the new test passes.
Feb 13 2018
Feb 7 2018
Added a test.
Feb 6 2018
Thanks, @compnerd! Updated to use your suggestion.
Update the tests to work with scalar parameter copies.
Prevent coroutine parameter stores from being moved, rely on https://reviews.llvm.org/D43000 instead.
Feb 1 2018
Oops, sorry, I should have accepted this revision along with my last comment. Thanks for making this change!
Oh I see! Thanks for the tip, I didn't know that.
Mostly looks good, but I had one suggestion!
Jan 28 2018
Add some diagnostics tests to SemaCXX/coroutines.cpp.
Prevent note diagnostics from being emitted for arity mismatch.
Jan 26 2018
Jan 24 2018
Great, thanks for the reviews!
Jan 18 2018
Friendly ping! Is this looking OK?
Jan 15 2018
Great, thanks for the review @bkramer!
Jan 14 2018
Thanks for the great review, @GorNishanov! You were exactly right, I had to remove the assert. I've taken all of your other suggestions as well. Let me know if anything else stands out at you. Also, thanks for the question, @EricWF, I added some comments to make it clear that this is an implementation of an experimental feature that has yet to be formally proposed.
Jan 13 2018
Great, thanks for the review, @aaron.ballman!
Yup, I totally agree, it would have been nice if in practice 'BC' was used by all LLVM IR bitstream formats. I wonder if it would possible, one day, to change the magic numbers for the major formats from Clang (and Swift) such that they do begin with 'BC'? Or are there too many clients that rely on the existing magic numbers? For example, does Apple's Xcode parse the magic numbers for serialized diagnostics somehow?
Jan 12 2018
Thanks for the reviews, everyone! I'll try to come up with something better here, and submit it in its own diff at a later date.
Add tests directly to the LLVM test suite. Thanks, @davide!
Very smart idea, @davide! I'll do that.
@davide I mentioned this in the "Test Plan" above, but I'm not sure how to test this without relying upon Clang. I do have a diff up for review that adds tests for this to the Clang repository, though: https://reviews.llvm.org/D41980. Maybe I'm missing something, though -- do you have any tips on how to test this entirely in the LLVM repo, so that I can include tests in this diff?
Jan 11 2018
Jan 10 2018
Thanks, will do next time :)
Jan 9 2018
Great, thanks for the review, @v.g.vassilev!
Great, thanks for the quick review, @jroelofs!