Page MenuHomePhabricator

[libc++] Make sure std::allocator<void> is always trivial
ClosedPublic

Authored by ldionne on Jun 16 2021, 10:08 AM.

Details

Summary

When we removed the allocator<void> specialization, the triviality of
std::allocator<void> changed because the primary template had a
non-trivial default constructor and the specialization didn't
(so std::allocator<void> went from trivial to non-trivial).

This commit fixes that oversight by giving a trivial constructor to
the primary template when instantiated on cv-void.

This was reported in https://llvm.org/PR50299.

Diff Detail

Event Timeline

ldionne requested review of this revision.Jun 16 2021, 10:08 AM
ldionne created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2021, 10:08 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added inline comments.Jun 16 2021, 10:13 AM
libcxx/include/__memory/allocator.h
83

@Quuxplusone I'd like you to double-check my reasoning here (see comment above). After thinking about it, I don't think this is an ABI break because of the subtlety where no two types are going to inherit from the same __non_trivial_if base class (unless they are the same type, in which case they *already* don't share the same address). So I think this is good, but an extra pair of eyes certainly won't hurt.

Also note that I would have used a trailing-requires-clause in C++20 instead of going through those base class hoops, but https://llvm.org/PR50740 prevents me from doing so.

Quuxplusone accepted this revision.Jun 16 2021, 11:30 AM
Quuxplusone added inline comments.
libcxx/include/__memory/allocator.h
83

Sure, LGTM. I haven't thought of any way for this to go wrong.

This revision is now accepted and ready to land.Jun 16 2021, 11:30 AM

Thanks, will ship once CI is green. I'll also cherry-pick this one to release/12.x.

This revision was automatically updated to reflect the committed changes.