- User Since
- Feb 11 2015, 3:26 PM (311 w, 10 h)
Tue, Jan 26
Mon, Jan 25
Thanks a lot Nico. The back story is that CI didn't run for that revision due to a misconfigured repository on the Phab revision. This is a great reminder that we should never skip pre-commit CI for non-trivial changes. This is my fault, I should have requested that the author updates the patch with a proper repository set so CI would trigger.
Ah, now I understand. This change is necessary to fix the build after https://reviews.llvm.org/D92044. The problem is that CI didn't run for that revision, so we didn't catch the failure. When you submit a patch, please make sure to follow the arc diff instructions here: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line.
Will commit once CI passes.
Can you please ping me once CI passes?
Thanks a lot! This looks good.
What's the status of this? Can you please rebase so CI runs (there were some CI issues during the holidays, I remember patch application failing). Ping me when this is ready for review, until then I'm marking as "changes requested".
Generally looks good, I'm just asking for a bit more manual testing. Thanks a lot for working on this, I love that we're removing dependencies on <random>.
I like this - I find it cleaner than a "private contract" with some specific users that they are allowed to overload __unwrap_iter. But I'd like to know why can't we leave that only enabled in C++20 mode. @rnk Would it be acceptable for Chrome to compile with C++20? It seems like an optimization that would justify the work to upgrade?
The P-numbered paper doesn't match what was actually landed in http://eel.is/c++draft/alg.shift .
LGTM once CI passes (after you silence the GCC warning). Ship it once it does!
Fri, Jan 22
Thanks a lot for doing this! This is great - historically we haven't been very good at communicating what we've done in libc++ from one release to another, but this is much better.
Thu, Jan 21
Please try this out and let me know if that solves your problems.
See tentative fix in https://reviews.llvm.org/D95177
This really isn't in my area, so I'm deferring to @compnerd . Please feel free to go ahead. :-)
Sorry this got through the cracks.
Generally speaking, I have no objection with doing this if it's an improvement. But I don't understand the benefits / tradeoffs here cause I don't have much experience with Clang modules (so far the libc++ Clang modules are kind of a side thing maintained by people who care about it, but we still use the non-modular build by default). Can you explain what introducing private modules is going to buy us, and what sort of changes are going to be required? Will this have any visible change for people building with -fcxx-modules or -fmodules today?
Wed, Jan 20
Per the discussion on the LWG reflector, this LGTM.
I would like to cherry-pick this change onto LLVM 11.1.0, if there is still time to do so. It is breaking Clang when the sysroot contains headers for libc++. Is it too late?
Fix broken link (I generated the documentation to test, this time).
Tue, Jan 19
Is this ready for review? If not, please ping me when it is. In the meantime, I'll "request changes" so it shows up correctly in the queue.
Thanks for your contribution!
In addition to the comments I made, can you please add tests using the type traits like static_assert(!std::is_copy_constructible_v<std::atomic<int>>);?
Re-generate feature-test macros after fixing the typo.
Note that I expect to have to make changes to the tests. I'm submitting a patch mainly to run the CI.
Gentle ping! Can we merge this? I'd love to move forward with https://reviews.llvm.org/D90257.
LGTM. Feel free to commit after fixing the minor comments.
Fix modules tests.
@tcanens You may want to consider updating cppreference in light of this patch when it gets in.
LGTM once CI passes!
Mon, Jan 18
I think allowing a non-copyable type to be returned makes more sense, however I think this needs a LWG issue so we can clear any doubts on the standardese. Are you willing to file it?
Thanks Arthur. Indeed, this doesn't apply cleanly on top of main. Please rebase and address Arthur's comments. Requesting changes just so it shows up in the review queue, but this LGTM except for what I just said.
Thanks for the review Zoe!
I'm not done with the review, but I figured I might as well give you the feedback I have right away.
@Quuxplusone Arthur, did you still have objections? If not, please accept and I'll commit this.
I didn't look again in detail, but I remember I was OK with the patch except for the question of whether to implement it as a DR. This has been settled now, so this LGTM. Thanks!
Do you need help committing this? If so, please provide Author Name <email@domain>. Thanks!
Do you need help committing this?