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?