Page MenuHomePhabricator

Support arrays in make_shared and allocate_shared (P0674R1)

Authored by ldionne on May 29 2019, 7:25 PM.



This patch implements P0674R1. It might be a bit rough around the edges, but I wanted to get it out so it can be reviewed and I can make changes to it as I straighten out the last few parts.

The main changes are how __shared_ptr_pointer deallocates itself and (obviously) the added overloads.

Thanks for the help @mclow.lists and @EricWF

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
zoecarver added inline comments.Feb 21 2021, 12:21 PM
47 ↗(On Diff #315705)

Done. Thanks :)

zoecarver marked an inline comment as done.
  • Address review comments.
  • Re-generate feature test macros after rebase.
  • Format changes with git-clang-format.
  • Poke buildkit.

OK, that seems to have worked. @ldionne for future reference: the CI was failing to apply this patch because I had two parent revisions and a child revision which were abandoned/already landed. I think the abandoned one was causing problems because it had conflicts. Removing those patches fixed the issue, but it would be nice to automatically skip abandoned parent patches.

  • Fix for -fno-exceptions.
  • Destroy alloc before deallocating this.
  • Cast char to size_t in assert.
zoecarver updated this revision to Diff 327001.Feb 28 2021, 3:13 PM
  • Fix UBSAN and GCC build bots.
zoecarver updated this revision to Diff 327002.Feb 28 2021, 3:14 PM
  • Fix ASAN bots.
  • Format test files.
zoecarver added inline comments.Feb 28 2021, 3:17 PM
10 ↗(On Diff #327002)

I really don't like disabling this whole test for sanitizer-new-delete. We need this because of the globalMemCounter tests. This is the same thing the unique_ptr tests use.

@ldionne do you know if there's a macro or some other way we can detect if we're running on sanitizer-new-delete? If so, we could just disable the two tests that actually cause this to fail.

ldionne added inline comments.Mar 1 2021, 10:32 AM
10 ↗(On Diff #327002)

I don't think there's such a macro. You could split it off into its own test file if you wanted.

zoecarver updated this revision to Diff 327614.Mar 2 2021, 4:04 PM
  • Fix UBSAN bot.
  • Fix macOS bot.
ldionne commandeered this revision.Oct 26 2021, 10:12 AM
ldionne edited reviewers, added: zoecarver; removed: ldionne.

Commandeering to finish this up.

What's the status of this one? Looks stalled, and I understand it's still a blocker for make_{unique,shared}_for_overwrite in D90111

What's the status of this one? Looks stalled, and I understand it's still a blocker for make_{unique,shared}_for_overwrite in D90111

This is rather complicated, but it's basically blocked on D114903.

ldionne updated this revision to Diff 405052.Feb 1 2022, 12:25 PM

Finish this. It turns out this is implementable without the construct_at extension for arrays.

Quuxplusone added inline comments.

IMO it would be cleaner to eliminate the partial specialization and just write

template <class _Array, class _Alloc>
struct __unbounded_array_control_block : __shared_weak_count
    using _Tp = remove_extent_t<_Array>;

Ditto for __bounded_array_control_block below. But admittedly I'd expect this to be marginally more expensive in terms of compile time.


__data_ is an array.


The condition could be __elements == 0 ? ~~~; the only singularity requiring a special case is at zero. It looks to me as if make_shared<int[]>(0) is intended to be UB: — but maybe we want to support it anyway for QoI, or maybe I'm misunderstanding the possibility of returning a pointer to an object of type U[0].


This isn't ADL-proof — add a regression test!
And then this can just be return __data_; because __data_ is an array, and then this function doesn't need to exist.
Ditto lines 1072, 1077, maybe elsewhere.


This should be =default...


...and this should be override (not virtual), IIUC.
(Ditto throughout.)


Why does this need to work with non-pointers at all? I'm 99% positive it should take _Tp* instead of an arbitrary _BidirIter. Likewise throughout — there are a lot of these unnecessarily bidirectional-iterator helpers.
Likewise line 363: using _ValueType = _Tp and then there's no more need for _ValueType to exist.

I'm not to familiar with this part of the Standard so didn't do an in-depth review. But I didn't see any oddities.


Let's guard against ADL-hijacking. (The required header is already included.)

ldionne updated this revision to Diff 418982.Mar 29 2022, 2:24 PM
ldionne marked 6 inline comments as done.

Address some comments. Updating to poke CI, I'll address remaining comments when I get another minute of free time.

Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2022, 2:24 PM
ldionne added inline comments.Mar 29 2022, 2:24 PM

I like the fact that we can confirm by inspection that the class is being instantiated on _Tp[] and not _Tp[N]. If I made it the base template, I would probably want to add a static_assert, but then I might as well use the status quo, which is simpler. I'm inclined not to change anything.


I know, but given that we're working with multidimensional arrays, it can become extremely confusing and I think the std::begin adds clarity, so I'm tempted to keep it.


I don't think this specific one is an ADL issue, because we're taking the address of the array, not the address of the first element (see But I'll still simplify this and stop taking the address, which is indeed not necessary. It *is* an issue in places below where we do &__data_[0] and I fixed it there and added tests.

I can't get rid of the method entirely without making __data_ public, which I'd rather not do, so I'll keep it around.


It turns out this doesn't work because __data_ is sometimes non-trivial, and a non-trivial member in a union results in a the destructor being deleted (which makes sense, because you might need to check which of the non-trivial members of the union needs to be deleted). I added a comment.

ldionne updated this revision to Diff 419151.Mar 30 2022, 8:18 AM
ldionne marked 2 inline comments as done.

Address review comments and fix modular CI. I'll ship this once CI passes.


I actually didn't find anything explicitly prohibiting make_shared<T[]>(0). Furthermore, I had never really thought about it, but based on it seems like dynamically-sized arrays of size 0 are definitely a thing and they are supported. Arrays with a static size 0 are not, though.

So yeah, I think make_shared<int[]>(0) & friends definitely need to work, and it must allocate an array with no members. I realized after reading this that I did have tests for this case already. I will add tests to ensure that we reject arrays with a static size of 0.

I reworded the comment and changed the condition to __elements == 0.


I don't see the benefit in removing abstraction when the cost of doing it is close to nothing. During development of the algorithm, abstracting to arbitrary iterators was incredibly useful, because at some point I was really confused -- these algorithms handle arrays specially, and arrays decay into pointers. Drawing a clear distinction between the iterator and the value type (which is sometimes an array) was very useful, and in fact there were some subtle bugs in the implementation before I decided to write it this way. So I'm pretty attached to the specific way those are written.

ldionne accepted this revision as: Restricted Project.Mar 30 2022, 8:18 AM
ldionne added inline comments.

Oh yeah, also I did check with other implementations:

ldionne updated this revision to Diff 419425.Mar 31 2022, 6:25 AM

Investigate CI failure on GCC.

ldionne updated this revision to Diff 419810.Apr 1 2022, 11:08 AM

Fix CI issues.

ldionne updated this revision to Diff 420161.Apr 4 2022, 6:34 AM

Poke CI.

ldionne updated this revision to Diff 420251.Apr 4 2022, 11:15 AM

Try to fix macOS CI.

ldionne updated this revision to Diff 420313.Apr 4 2022, 2:44 PM

Rebase on top of the is_array fix.

ldionne updated this revision to Diff 420496.Apr 5 2022, 7:06 AM

Try to fix CI issues on AIX and GCC.

ldionne updated this revision to Diff 420657.Apr 5 2022, 4:42 PM

Try to fix CI again

Finally! Shipping this now.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 6 2022, 5:43 AM
This revision was automatically updated to reflect the committed changes.