Details
- Reviewers
ldionne Mordante var-const huixie90 - Group Reviewers
Restricted Project - Commits
- rG79df8e19beb9: [libc++] Implement P0591R4 (Utility functions to implement uses-allocator…
rG099384dcea49: [libc++] Implement P0591R4 (Utility functions to implement uses-allocator…
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Please go through D58879 and reuse anything useful (esp. from the tests). Including comments like https://reviews.llvm.org/D58879#1546317. Then we can close D58879 since this supersedes it.
libcxx/include/__memory/uses_allocator_construction.h | ||
---|---|---|
31 | Maybe this should be three functions instead, it would be easier to read. | |
151 | FYI this seems to be the resolution of http://wg21.link/LWG3525. I assume now you know how to test it? |
libcxx/include/__memory/uses_allocator_construction.h | ||
---|---|---|
23 | I am slightly confused by the versions. The paper seems to be C++20, but all the internal __ functions are targeting C++17, and then the tests are running on all language versions? | |
52 | As you mentioned in another thread, should be use int = __enable_if_t<b, int> = 0 instead through out the change? | |
54 | is noexcept missing here? Is there a test? | |
88 | I believe this is part of the zip paper, which is c++23. perhaps you need to branch under different language versions? | |
112 | I believe this is part of the "zip" paper and perhaps you can update the zip project csv as well. I have a patch related to pair ongoing as well which skipped this part | |
123 | nit: can we change its name to be more unique? the chance of other people using the name __detail::__fun is not zero | |
132 | inline | |
152 | nit: what does Mut mean? | |
168 | does parenthesis work for c++17 to init aggregates? | |
libcxx/test/std/utilities/memory/allocator.uses/allocator.uses.construction/uses_allocator_construction_args.pass.cpp | ||
63 | why maybe_unused? it seems that you are using the result in most of the tests |
- Address comments
libcxx/include/__memory/uses_allocator_construction.h | ||
---|---|---|
23 | polymorphic_allocator uses uses-allocator-construction, which this implements. So instead of implementing it twice we can use the same implementation. The tests were just an oversight. | |
31 | I read the standard again, and the program is actually ill-formed if none of the if constexpr conditions are met. | |
52 | Yes. I wrote this before the discussion. Thanks for not letting me produce more work for myself! | |
54 | Good catch! Added a regression test. | |
152 | mutable | |
168 | Nope, replaced it with {}. | |
libcxx/test/std/utilities/memory/allocator.uses/allocator.uses.construction/uses_allocator_construction_args.pass.cpp | ||
63 | Because I forgot to remove it. |
Leaving final approval to @huixie90
libcxx/include/__memory/uses_allocator_construction.h | ||
---|---|---|
152 | I think Mutable would be clearer, especially given the question. I had the same. | |
libcxx/test/std/utilities/allocator.adaptor/allocator.adaptor.members/construct_pair.pass.cpp | ||
56 | Would it be valid to pass it as a const& before C++20? |
Almost LGTM. Just two small clarification questions
libcxx/include/__memory/uses_allocator_construction.h | ||
---|---|---|
132 | question : do we need hidden from abi macro for inline constexpr variables | |
172 | In your original version , this is doing sfinae, but now you’ve changed the use_allocator_constructution_args to static_assert in the function body. Does this still do the things you want? |
libcxx/include/__memory/uses_allocator_construction.h | ||
---|---|---|
132 | We don't do it anywhere else and it would also kind-of defeat the purpose of marking it inline. I don't think it will ever be emitted anyways, since AFAIK the only time clang will emit the symbol is when you take the address of it, which we will never do. | |
172 | Yes. The instantiation error is actually requested by the standard, so this fails correctly. |
- Address comments
libcxx/test/std/utilities/allocator.adaptor/allocator.adaptor.members/construct_pair.pass.cpp | ||
---|---|---|
56 | I'll look into that after this patch. If it's the case I'll create a follow-up to fix the tests. |
I am slightly confused by the versions. The paper seems to be C++20, but all the internal __ functions are targeting C++17, and then the tests are running on all language versions?