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

[philnik]

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.

[ldionne]

libcxx/include/__memory_resource/polymorphic_allocator.h

65 ↗

(On Diff #398211)

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 ↗

(On Diff #398211)

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 ↗

(On Diff #398211)

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

libcxx/include/__memory_resource/unsynchronized_pool_resource.h

105–109 ↗

(On Diff #398211)

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

21

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 ↗

(On Diff #398211)

[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 ↗

(On Diff #398211)

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

156 ↗

(On Diff #398211)

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

2

No file name!

14

Please indent nested #includes.

29

Why not remove this altogether?

33

Let's always include this.

54

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.

112

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 ↗

(On Diff #398211)

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

11

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

13–21

Sort?

libcxx/utils/generate_feature_test_macro_components.py

316

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

368

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

388–389

471–472

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

43–49

Please reorder the includes

57–59

74–75

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

27–32

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

29–40

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

43

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

40

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

68–72

[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.

[philnik]

• Rebased
• clang-formatted
• Replaced _VSTD with std

[philnik]

• Try to fix CI

[philnik]

• Fix transitive includes list

[philnik]

poke CI

[ldionne]

LGTM once the CI passes.

libcxx/lib/abi/CHANGELOG.TXT

144 ↗

(On Diff #462232)

This should be moved to 16.0.

[philnik]

poke CI

[philnik]

• Try to fix CI

[philnik]

• Next try

[philnik]

• Update ABI lists

[philnik]

• Rebased
• Fix transitive_includes tests
• XFAIL macOS

[philnik]

• Generate files

[ldionne]

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

2

Please rename this to debug.deallocate.pass.cpp for consistency.

10–12

[philnik]

• Try to fix CI

[philnik]

• Next try

[ldionne]

libcxx/lib/abi/CHANGELOG.TXT

146–147 ↗

(On Diff #464193)

149–189 ↗

(On Diff #464193)

[philnik]

• Rebased
• Address comments