Page MenuHomePhabricator

Implement C++17 <memory_resource>.
AbandonedPublic

Authored by Quuxplusone on May 18 2018, 3:19 PM.

Details

Summary

This is a complete but partly silly implementation of C++17 <memory_resource>.

Preliminary pieces of this patch are already filed as D46806 and D46807. If those get merged I'll rebase this one. I myself do not have commit privileges.

Potential issues with this patch:

  1. monotonic_buffer_resource is complete and IMHO well tested, but I bet someone can find suboptimal things in its design.
  1. unsynchronized_pool_resource is trivial; it ignores the pool_options and just passes everything through to the upstream, which is conforming but I'm sure someone has a better idea.
  1. unsynchronized_pool_resource is "code complete" but untested.

I feel guilty about (3) and will fix it eventually. I'm willing to take a stab at (2) if anyone can explain in English what the _intended_ implementation is. On (1) I hope there will be only easy nits.

https://github.com/Quuxplusone/libcxx/compare/master...memory-resource-header

Diff Detail

Repository
rCXX libc++

Event Timeline

Quuxplusone created this revision.May 18 2018, 3:19 PM
Quuxplusone edited the summary of this revision. (Show Details)May 18 2018, 3:21 PM

I would prefer if we completed <experimental/memory_resource> (according to the current standard, not the LFTS spec), and then moved it.

Would you be willing to do that instead?

I would prefer if we completed <experimental/memory_resource> (according to the current standard, not the LFTS spec), and then moved it.
Would you be willing to do that instead?

Let me see if I understand. libc++'s <experimental/memory_resource> differs from C++17 <memory_resource> in at least these ways:
(A) It's missing monotonic_buffer_resource and {un,}synchronized_pool_resource
(B) It's got different semantics around uses-allocator construction because of https://wg21.link/lwg2969
(C) It's got a different header name

This patch is basically proposing to fix things in the order C-A-B (and fix C by making copies of everything); you're proposing to fix things in the order A-B-C (and fix C by moving everything).

I have no objection to fixing A in <experimental/memory_resource> if people think that'd be useful. I didn't do that in this patch simply because I'd observed other <experimental/*> headers already getting replaced with #error messages, and it seemed like any further work on the <experimental/*> headers would have been wasted.

I don't know who's using <experimental/memory_resource> today, but in theory I worry that fixing B in <experimental/memory_resource> (before doing C) might actually break someone's code.

Philosophically as I understand it LWG2969 was a patch against C++17, not a patch against the TS. If LWG2969 (and incidentally I asked lwgchair to open a new issue for https://github.com/Quuxplusone/libcxx/commit/54e1a59a ) are actually supposed to be patches against *both* C++17 *and* the TS, then cool, yeah, we should fix B in <experimental/memory_resource>. Thoughts?

I would prefer if we completed <experimental/memory_resource> (according to the current standard, not the LFTS spec), and then moved it.
Would you be willing to do that instead?

Let me see if I understand. libc++'s <experimental/memory_resource> differs from C++17 <memory_resource> in at least these ways:
(A) It's missing monotonic_buffer_resource and {un,}synchronized_pool_resource

Those were initially in the LFTS spec, I think they're in LFTS v2.

(B) It's got different semantics around uses-allocator construction because of https://wg21.link/lwg2969

Issue resolutions should probably be applied to the experimental versions as well.

(C) It's got a different header name

I don't think this is relevant.

This patch is basically proposing to fix things in the order C-A-B (and fix C by making copies of everything); you're proposing to fix things in the order A-B-C (and fix C by moving everything).

I have no objection to fixing A in <experimental/memory_resource> if people think that'd be useful. I didn't do that in this patch simply because I'd observed other <experimental/*> headers already getting replaced with #error messages, and it seemed like any further work on the <experimental/*> headers would have been wasted.

I don't know who's using <experimental/memory_resource> today, but in theory I worry that fixing B in <experimental/memory_resource> (before doing C) might actually break someone's code.

I'm not concerned with it. We make no promises about ABI and API stability for <experimental> headers. Do we implement the LFTS v1, v2, or in future, v3, API? I would rather follow the standard spec. I believe
it will actually make transitioning from <experimental/foo> to <foo>, since the behaviour is the same.

I should add that this is the approach I've taken with <experimental/filesystem> with no complaints.

src/memory_resource.cpp
63

We certainly don't want a different definition of the global resources in each TU. See below.

65

This should go inside the library, so we're not emitting an initializer in each TU.

84

I think the global resources should be in the unversioned namespace std::pmr instead of std::__v::pmr. Like new/delete and exceptions I suspect we only want one definition of these in any given program. I'm considering every function from here up to and including set_default_resource.

@mclow.lists, @dexonsmith: what do you think?

148

_Tp

150

__res_
__ptr_
__size_
__align_

181

__bytes_
__alignment_ (or __align_)
__allocation_
__next_

Quuxplusone added inline comments.May 19 2018, 11:39 AM
include/__memory_resource_base
197

(B) It's got different semantics around uses-allocator construction because of https://wg21.link/lwg2969

Issue resolutions should probably be applied to the experimental versions as well.

Okay, I can roll with that. I'll create a new patch that modifies <experimental/memory_resource>'s uses-allocator parts per LWG2969 (and my followup DR), without any other diffs.

Should <experimental/memory_resource> continue caring about std::experimental::erased_type, which was in LFTS and LFTSv2 but did not make it into C++17? My kneejerk reaction is "yes". (And at the end of this process, when we copy <experimental/memory_resource> into <memory_resource>, should <memory_resource> care about erased_type? My kneejerk reaction is "no".)

src/memory_resource.cpp
63

@EricWF: I think all of your comments in this file are the result of misreading "src/memory_resource.cpp" as "include/memory_resource". Or else I *totally* don't understand the current directory organization and you're going to have to instruct me. :)

EricWF added inline comments.May 19 2018, 11:42 AM
src/memory_resource.cpp
63

Woops. I feel dumb now.

Here's a work-in-progress, slightly-less-silly implementation of {un,}synchronized_pool_resource, plus a test.

Eric has suggested adding these things into <experimental/memory_resource> instead of <memory_resource>, and then creating <memory_resource> as the very last step; so I've started opening pull requests in that direction already (D46806, D47109) and will keep doing so, as those start getting merged.

Quuxplusone abandoned this revision.May 24 2018, 11:50 PM

I'm re-submitting this as a series of smaller patches that first bring <experimental/memory_resource> up to date with C++17, and then copy it over to <memory_resource>.
In order, these smaller patches are: D46806 D47109 D47344 D47111 D47358 D47360.