This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement P0591R4 (Utility functions to implement uses-allocator construction)
ClosedPublic

Authored by philnik on Aug 15 2022, 8:22 AM.

Diff Detail

Event Timeline

philnik created this revision.Aug 15 2022, 8:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2022, 8:22 AM
Herald added a subscriber: mgorny. · View Herald Transcript
philnik requested review of this revision.Aug 15 2022, 8:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2022, 8:22 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Aug 18 2022, 9:46 AM

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?

This revision now requires changes to proceed.Aug 18 2022, 9:46 AM
huixie90 requested changes to this revision.Aug 18 2022, 1:01 PM
huixie90 added a subscriber: huixie90.
huixie90 added inline comments.
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
https://reviews.llvm.org/D131495

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

philnik updated this revision to Diff 454511.Aug 22 2022, 8:27 AM
philnik marked 12 inline comments as done.
  • 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.

philnik updated this revision to Diff 457044.Aug 31 2022, 11:45 AM
  • Fix CI; update scoped_allocator_adaptor
ldionne accepted this revision as: ldionne.Sep 2 2022, 9:29 AM

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

171

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?

philnik added inline comments.Sep 5 2022, 8:58 AM
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.

171

Yes. The instantiation error is actually requested by the standard, so this fails correctly.

philnik updated this revision to Diff 458057.Sep 5 2022, 3:27 PM
  • Try to fix CI
huixie90 accepted this revision.Sep 6 2022, 5:25 AM
This revision is now accepted and ready to land.Sep 6 2022, 5:25 AM
philnik updated this revision to Diff 458196.Sep 6 2022, 9:38 AM
philnik marked 3 inline comments as done.
  • 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.

philnik updated this revision to Diff 461000.Sep 17 2022, 6:05 AM
  • Try to fix CI
philnik updated this revision to Diff 464195.Sep 30 2022, 2:42 AM
  • Try to fix CI
vitalybuka reopened this revision.Oct 2 2022, 6:41 PM
This revision is now accepted and ready to land.Oct 2 2022, 6:41 PM
philnik updated this revision to Diff 465033.Oct 4 2022, 8:55 AM
  • Fix includes

Landing this again, since the CI is green with the updated transitive includes test.

libcxx/test/std/utilities/memory/allocator.uses/allocator.uses.construction/make_obj_using_allocator.pass.cpp