- User Since
- Feb 11 2015, 3:26 PM (310 w, 1 d)
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?
Address Zoe's comments. Note that this is meant to be a NFC commit, so I don't
think we should incorporate additional changes beyond the refactorign to those
Fix test on GCC and without localization support.
Please fix the CI issues and ping again if you'd like to get this reviewed.
I still don't understand how things work if we #undef those macros. If getc was a macro, then arguably ::getc (after undefining the macro) isn't pointing to a valid declaration, unless they decided to provide both a function ::getc *and then* to define a macro with the same name. How does that work? @brad
If you say it's monotonic..
This is why i think that's okay (from the documentation you linked on Dec 9):
This LGTM, however I'll wait until the follow-up patches have been submitted to approve this. Can you please upload whatever patches depend on this and mark them as children of this one? If the follow-up patches make sense, then I have no objection with this.
Thanks for your contribution. However, it would be useful to have more information before going forward. The goal is to avoid adding random bits of complexity to the library for a system that isn't officially supported and maintained.
Add -fPIC when compiling the tests.
Fri, Jan 15
Add comment to explain the purpose of the unit test.
Add test, fix potential for duplicate macro definition.
LGTM, but we should really fix the clang bug. Please wait for CI to finish before committing.
Thu, Jan 14
See the discussion in https://reviews.llvm.org/rG1a92de00 for more context.
By the way, I think it is actually the following commit that broke you:
LGTM if CI passes. I think you need to re-upload the diff for it to trigger, since the LLVM Monorepo tag was somehow removed.
That seems correct to me: http://eel.is/c++draft/thread.mutex#requirements.mutex.general-14