Page MenuHomePhabricator

Update shared_ptr's constructor
Needs ReviewPublic

Authored by zoecarver on Aug 13 2019, 3:13 PM.

Details

Summary

This patch represents the ethos of D62233. It is much smaller and will hopefully be easier to review while still achieving the same result :)

Event Timeline

zoecarver created this revision.Aug 13 2019, 3:13 PM

Another friendly ping :) I would like to get this going because other patches rely on it. I have patches to resolve bugs, issues, and features. @mclow.lists @EricWF @ldionne

Weekly ping ;)

Ping. Thanks for reviewing the other patches, this is the last in the series.

ldionne requested changes to this revision.Oct 22 2019, 12:15 PM
ldionne added inline comments.
libcxx/include/memory
3982

Before, we would create the control block with new, and with this patch we're using the allocator. Is this correct? Why?

Also, we're now unconditionally using a try-catch block -- I'd be curious to see what the impact on code generation is.

4012

Note to reviewers: It's okay to move this outside of the try-catch block, since __enable_weak_this is noexcept anyway.

This revision now requires changes to proceed.Oct 22 2019, 12:15 PM
zoecarver marked an inline comment as done.Oct 22 2019, 3:48 PM
zoecarver added inline comments.
libcxx/include/memory
3982

Yes, it's correct. The allocator will call __builtin_operator_new (or operator new), which makes it functionally equivalent. When the control block is deallocated, it calls __a.deallocate. I think it's better that the control block is also allocated with the allocator (even if they're functionally equivalent) both for cognitive load and in case we introduced an optimization (for example) that broke this type of deallocation down the road.

I'll take a look at the codegen.

zoecarver marked an inline comment as done.Oct 22 2019, 3:58 PM
zoecarver added inline comments.
libcxx/include/memory
3982

Yes, the try-catch-block really hurts the codegen. With and without (18 vs 2 lines of assembly).

zoecarver marked an inline comment as done.Oct 22 2019, 4:01 PM
zoecarver added inline comments.
libcxx/include/memory
3930

I'm going to move the try-catch-blocks out of these two members and into the initializers they're used in. This should help with codegen.

Also, I forgot to wrap these in the #ifs.

zoecarver updated this revision to Diff 230326.Nov 20 2019, 1:55 PM
zoecarver retitled this revision from Update shared_ptr's constructor to Update shared_ptr's constructor.
zoecarver edited the summary of this revision. (Show Details)
  • Move try-catch outside of __allocate_shared
Herald added a project: Restricted Project. · View Herald TranscriptNov 20 2019, 1:55 PM