Add the C++17 <memory_resource> header (mono-patch)

[• Quuxplusone]

Summary

This patch is the rebase and squash of three earlier patches. It supersedes all three of them.

• D47111: experimental monotonic_buffer_resource.
• D47358: experimental pool resources.
• D47360: Copy std::experimental::pmr to std::pmr.

The significant difference between this patch and the-sum-of-those-three is that this patch does not add std::experimental::pmr::monotonic_buffer_resource and so on. This patch simply adds the C++17 standard facilities, and leaves the std::experimental namespace entirely alone.

[jloser]

@Quuxplusone can you please remove destroy() (and its tests) from polymorphic_allocator? Then you can mark LWG3036 as done and we avoid the issue of whether to mark destroy() as deprecated vs hard removal. Simply not including it when we ship this for the first time is the easiest :)

[• Quuxplusone]

polymorphic_allocator: Remove the __max_size() private member, and include-what-you-use. Mark the allocate method as [[nodiscard]] (to match C++20). Stop discarding the result of allocate in the tests. Mark destroy as _LIBCPP_DEPRECATED_IN_CXX23. Stop using destroy in the tests.

I've now got a local patch on top of this one that implements p0339 "polymorphic_allocator<> as a vocabulary type," but I still need to write unit tests for it.

libcxx/include/__memory_resource/polymorphic_allocator.h

147–150

@jloser wrote:

@Quuxplusone can you please remove destroy() (and its tests) from polymorphic_allocator? Then you can mark LWG3036 as done and we avoid the issue of whether to mark destroy() as deprecated vs hard removal. Simply not including it when we ship this for the first time is the easiest :)

Sadly, it appears that destroy is only deprecated in C++23, not removed, so if we want to be conforming we should still have it (just marked as [[deprecated]]).
http://eel.is/c++draft/depr#mem.poly.allocator.mem

Or is your point that we could just kinda "forget" to implement destroy (the same way we've "forgotten" to implement all of <memory_resource> for the past 4 years ;)), and just wait for the Standard to catch up to our non-implementation? @ldionne thoughts?

[jloser]

libcxx/include/__memory_resource/polymorphic_allocator.h

147–150

Yeah, I was hoping we could skip the deprecated step since libc++ doesn't have std::pmr::polymorphic_allocator right now - it's in the experimental namespace.

[dougsonos]

libcxx/include/__memory_resource/polymorphic_allocator.h

147–150

By this same logic, libc++ could neglect to implement all the variants of construct() which were added in C++17 and removed in C++20 :/

[philnik]

I didn't get all the way through, but here are about 20 nit picks. I'll look at the tests later.

libcxx/include/__memory_resource/memory_resource.h

30

Is there a reason this isn't constexpr or just directly in lines 35 and 39?

37

I hate this brace style with all my heart, but it's used through out the code base so I guess I can't do anything about it.

40

Nit: Use void* instead of void * or at least do it consistently (see line 35).

69–79

libcxx/include/__memory_resource/synchronized_pool_resource.h

62

Remember https://youtu.be/F6Ipn7gCOsY?t=1835?

libcxx/include/__memory_resource/unsynchronized_pool_resource.h

50–58

Again - is there a reason these aren't constexpr?

97

What do you want to say with // key function? For the vtable?

libcxx/include/deque

3027–3035

Why is this not just in the above std namespace?

libcxx/lib/abi/CHANGELOG.TXT

26–60

Are these symbols not added under linux?

libcxx/src/memory_resource.cpp

18

Doesn't this only work with lld?

117–118

127–136

And with the other set checks as well.

128

177–178

191–192

213

Wouldn't it make more sense to put that in the header so the dylib doesn't have to change depending on wanting these assertions or not?

214–229

336–346

414

Please remove this else as well.

427–428

Why not use std::min and std::max?

[• Quuxplusone]

Address @philnik's review comments.
Mark allocate as [[nodiscard]]: that might be new in C++20, but I think we can get away with backporting it anyway. Should add a test for that, I guess.

libcxx/include/__memory_resource/memory_resource.h

30

The answer in both cases is that this came via copy-paste from <experimental/memory_resource>. I agree s/const/constexpr/g is an improvement. I'll keep it pulled out as a named constant, though, since it's used in two places and it's just barely conceivable that some platform might like to change it.

40

This is one of those places where I think LLVM style is T* x but my-preference and at-least-half-the-time-libc++-style is T *x. I'll change line 35 instead.

libcxx/include/__memory_resource/synchronized_pool_resource.h

62

:) I actually have no idea why I did this here. Will fix.

libcxx/include/__memory_resource/unsynchronized_pool_resource.h

97

Yes, that's right; see also

../libcxx/include/__filesystem/filesystem_error.h:  ~filesystem_error() override; // key function

libcxx/include/deque

3027–3035

Here and also in the C++20 Ranges stuff, I've got a habit of "returning to zero" in between unrelated facilities, i.e. I'll nest things like (x)((y))(x) rather than (x(y)x). IMO this makes it easier to keep track of the nesting levels. It's not to the level of a conscious guideline with me, necessarily — it's mostly muscle memory / personal style — but if you explicitly ask me to pick one style or the other, well, I'll still pick this one.
Leaving as-is, at least until a consensus builds against it. ;)

libcxx/lib/abi/CHANGELOG.TXT

26–60

They are; I just was too lazy to dig up the build artifact that lists them. Will fix after a build completes again.

libcxx/src/memory_resource.cpp

18

Copied from experimental/memory_resource.cpp, so it'll work as well as that one does (for better and worse).

127–136

This was originally copy-pasted from src/experimental/... but the early-return rewrite strikes me as harder to read, so I'm keeping it as-is for now.

128

PSA: You flipped the sense of this; you meant != nullptr. Fixed.

213

I agree with your goal, but this guy's caller is unsynchronized_pool_resource::do_deallocate (also in src/), so I don't think it's possible. Not without leaking a lot more stuff into the headers, which conflicts with our other goal of trying to maintain some semblance of ability-to-change-implementation.

427–428

Originally, either because <algorithm> had not been granularized and I didn't want to drag in all of <algorithm> for this (although that's a bad rationale), or because these can deal with mixed types (e.g. min(sizeof(int), 4) whereas _VSTD::min would require casting.
But now: sure, OK.

449–452

FYI, this is the call that requires a cast after switching to _VSTD::min.

[ldionne]

libcxx/docs/Status/Cxx20Issues.csv

236

This should be 15.0 since we slipped again.

libcxx/docs/Status/Cxx2bIssues.csv

19

Same, 15.0.

libcxx/include/__memory_resource/polymorphic_allocator.h

65

Please use numeric_limits<size_t>::max(). I know they are equivalent, but being close to the Standard is good. I don't mind taking an additional dependency on <limits> at all. This applies elsewhere too.

153–155

This is nitpicky on a style matter, but can you please just do

memory_resource *resource() const noexcept { 
    return __res_;
}

or

memory_resource *resource() const noexcept { return __res_; }

if you really care about saving a line. This applies elsewhere too.

The reason I'm asking this is that we don't use the pattern you have here anywhere else (that I can tell), and I don't want to introduce yet another stylistic inconsistency.

libcxx/include/__memory_resource/synchronized_pool_resource.h

18

Please indent includes when they are nested to near-located #ifs.

libcxx/include/__memory_resource/unsynchronized_pool_resource.h

105–109

It sucks that we have to commit to an ABI right here. It would be great if we could instead store a pimpl, but that would require allocating and that's not a great idea.

libcxx/include/memory_resource

2

We don't include the name of the file anymore!

20

Can we get section names like // [mem.res.class]?

libcxx/include/regex

6801

Any reason for not putting those declarations inside the _LIBCPP_BEGIN_NAMESPACE_STD that was closed just above?

6805

Suggestion: you don't have to, but you could move all your new code to std:: instead of _VSTD::.

libcxx/include/string

4593

I think this needs to be guarded by _LIBCPP_HAS_NO_WIDE_CHARACTERS. I would expect this to be caught by the CI.

libcxx/lib/abi/CHANGELOG.TXT

1

[commenting here for lack of better place]

Another thing we'll need to add is availability macros for the newly added dylib functionality. You can look at include/__availability for how that's done.

111

This should be moved under a new section 15.0. Sorry for the churn.

156

You can figure this one out by running ./libcxx/utils/ci/run-buildbot-container locally (requires Docker) and then running the generate-cxx-abilist target on Linux.

libcxx/src/memory_resource.cpp

1

No file name!

13

Please indent nested #includes.

28

Why not remove this altogether?

32

Let's always include this.

53

So when aligned allocation is not supported, we allocate unaligned and try to align the pointer within the allocated buffer. If that works, we're happy, otherwise we fail. I don't understand why that would ever succeed, can you explain?

Also, I've always been a bit confused by the _LIBCPP_HAS_NO_ALIGNED_ALLOCATION situation. It would be worth looking into whether we can just assume that it's provided when we build the library.

111

Is there a reason why you don't just assume that atomics are available? I would do:

#if defined(_LIBCPP_HAS_NO_THREADS)

    <no threading implementation>

#else

    <use atomic>

#endif

libcxx/src/memory_resource_init_helper.h

2

Once you rebase, you'll need to use _LIBCPP_CONSTINIT.

libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/resource.pass.cpp

10

Here and everywhere else, you'll need to also add

// XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx{{11|12}}

libcxx/test/support/test_std_memory_resource.h

12–20

Sort?

libcxx/utils/generate_feature_test_macro_components.py

460

Can we add a release note?

[philnik]

Could you add comments in the TODO.txt for all the experimental things that can be removed in libc++17 (hopefully)? I think most experimental headers could be removed.

libcxx/src/memory_resource.cpp

367

It's not a strong feeling, but I'd rather see *= 2 than <<= 1. FWIW it produces the same asm, even at -O0

387–388

470–471

libcxx/test/libcxx/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/construct_piecewise_pair.pass.cpp

42–48

Please reorder the includes

56–58

73–74

libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.ctor/copy.pass.cpp

26–31

libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.ctor/other_alloc.pass.cpp

28–39

libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/construct_pair.pass.cpp

42

Couldn't we just use a char ptr[sizeof(P)]? Seems overkill to malloc() for that.

libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/construct_pair_rvalue.pass.cpp

39

Please choose PtrT* v or PtrT *v, but not PtrT * v for all that is sacred. I don't want to multiply my types!

libcxx/test/std/utilities/utility/mem.res/mem.res.global/default_resource.pass.cpp

67–71

[mmuetzel]

Is this still being worked on? It would be nice (from a user point of view) if this could be in one of the next releases of libc++.

(I'm sorry for the noise if bumping dists like this is frowned upon.)

[Mordante]

In D89057#3595320, @mmuetzel wrote:

Is this still being worked on? It would be nice (from a user point of view) if this could be in one of the next releases of libc++.

I'm not aware of anybody working on this so I don't expect it to become part of the next libc++ release.
I'm not going to work on it, my todo list has enough items to keep me busy.

[philnik]

In D89057#3597000, @Mordante wrote:

In D89057#3595320, @mmuetzel wrote:

Is this still being worked on? It would be nice (from a user point of view) if this could be in one of the next releases of libc++.

I'm not aware of anybody working on this so I don't expect it to become part of the next libc++ release.
I'm not going to work on it, my todo list has enough items to keep me busy.

I plan to work on it after the LLVM 15 release branch if we don't get things sorted with Arthur until then. I really want to get this into LLVM 16, but who knows how long it will take.