This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Fix __compressed_pair so it doesn't copy the argument multiple times, and add constexpr.
ClosedPublic

Authored by EricWF on Dec 7 2016, 8:37 PM.

Details

Summary

__compressed_pair takes and passes it's constructor arguments by value. This causes arguments to be moved 3 times instead of once. This patch addresses that issue and fixes constexpr on the constructors.

I would rather have this fix than D27564, and I'm fairly confident it's not ABI breaking but I'm not 100% sure.

I prefer this solution because it removes a lot of code and makes the implementation *much* smaller.

Diff Detail

Event Timeline

EricWF updated this revision to Diff 80714.Dec 7 2016, 8:37 PM
EricWF retitled this revision from to [libcxx] Alternative fix for compressed pair.
EricWF updated this object.
EricWF added a reviewer: mclow.lists.
EricWF updated this object.
EricWF added a subscriber: cfe-commits.
EricWF updated this revision to Diff 80715.Dec 7 2016, 8:40 PM

Remove ever more dead code.

EricWF added a comment.Dec 7 2016, 9:04 PM

Grr... OK so this is currently ABI breaking because it re-arranges the layout of __compressed_pair<T1, T2> when T2 is empty but T1 is not. I might be able to work around this with some metaprogramming.

EricWF updated this revision to Diff 93991.Apr 3 2017, 7:26 PM
EricWF retitled this revision from [libcxx] Alternative fix for compressed pair to [libcxx] Fix __compressed_pair so it doesn't copy the argument multiple times, and add constexpr..
EricWF edited the summary of this revision. (Show Details)

I'm going ahead with this patch instead of https://reviews.llvm.org/D27564

EricWF added a subscriber: K-ballo.

@K-ballo if you have time would you do me the favor and looking this over.

In particular that I didn't accidentally change the layout of __compressed_pair. I don't think I did but I need a second set of eyes.
[Here is a standalone header containing the old __compressed_pair implementation](https://gist.github.com/EricWF/554eea08b62148ff56ae84e7d32e1fbb), because it's harder to read the diff.

K-ballo edited edge metadata.Apr 4 2017, 6:38 PM

I don't see any potential layout change, other than the case in which both T1 and T2 are empty (which I understand doesn't happen within libc++). As far as I know, the rest of the cases should result in the same layout regardless of whether EBO actually applies or not.

Some notes inline.

include/memory
2082

Should be _IsEmpty

2087

If _Tp can actually be a reference type, turning it into reference to const here doesn't seem right

2125

This use of __value_ as a type is overly smart, consider just using _Tp for clarity

EricWF updated this revision to Diff 95051.Apr 12 2017, 4:16 PM
EricWF marked 2 inline comments as done.
  • Address all inline comments.
EricWF marked an inline comment as done.Apr 12 2017, 4:17 PM
EricWF added inline comments.
include/memory
2082

Changed to _CanBeEmptyBase.

2087

Agreed. Fixed by correctly collapsing T& -> T& instead of T const&

2125

Changed to __value_type instead.

K-ballo accepted this revision.Apr 12 2017, 4:56 PM

LGTM

This revision is now accepted and ready to land.Apr 12 2017, 4:56 PM
EricWF updated this revision to Diff 95058.Apr 12 2017, 4:57 PM
EricWF marked an inline comment as done.
  • Add a static assertion that the new __compressed_pair<T1, T2> implementation is never used in a way that would be ABI-incompatible with the previous one. Specifically add a static assert that T1 != T2. This case should never arise because libc++ never uses __compressed_pair in a place where both element types should be the same. However assert that it doesn't occur just to be explicit.
EricWF closed this revision.Apr 12 2017, 4:58 PM