Page MenuHomePhabricator

Use allocator_traits to consistently allocate/deallocate/construct/destroy objects in std::any
ClosedPublic

Authored by ldionne on Jun 3 2020, 11:28 PM.

Details

Reviewers
tkoeppe
mclow.lists
Group Reviewers
Restricted Project
Commits
rG39c879514170: [libc++] Use allocator_traits to consistently…
Summary

PR45099 notes (correctly) that we're inconsistent in memory allocation in std::any. We allocate memory with std::allocator<T>::allocate, construct with placement new, destroy by calling the destructor directly, and deallocate by calling delete. Most of those are customizable by the user - but in different ways.

The standard is silent on how these things are to be accomplished.
This patch makes it so we use allocator_traits<allocator<T>> for all of these operations (allocate, construct, destruct, deallocate). This is, at least, consistent.

Diff Detail

Event Timeline

mclow.lists created this revision.Jun 3 2020, 11:28 PM
mclow.lists edited the summary of this revision. (Show Details)
EricWF added a subscriber: EricWF.Jun 4 2020, 1:12 AM

std::any, like other type erased types, cannot follow the allocator model and therefore does not use allocators.

The behavior of a user-provided specialization of allocator or allocator traits cannot differ meaningfully from the current code.
So I don't understand what the observable difference is here. Could you add a test?

libcxx/include/any
370

Can std::allocator have behavior that observably differs from this code? (which is just the default std::allocator inlined).

382

Is it the case that the allocator which allocated this memory is always equal to a default constructed allocator? If not, this is UB.

EricWF added a comment.Jun 4 2020, 1:17 AM

All of that being said, this code is correct and LGTM**.

  • Assuming the default constructed allocate is always equal to the allocator user to allocate the memory.

So I don't understand what the observable difference is here. Could you add a test?

An observable difference:

The user has specialized std::allocator for his type (and it is 'large' for anys purposes).
When he puts one into an any, his allocator specialization is called to allocate the memory.
When the any is destroyed (or he puts something else into the any), then the global ::free is called.
This is potentially UB, since it passes free a pointer that didn't come from new.

There's a bunch of ways to do this. We should be consistent.
This is one way to be consistent. Another would be to use new/free everywhere.
I'm not really sure which is best. Since we were using the allocator before, I made it so we used it everywhere.

mclow.lists marked an inline comment as done.Jun 4 2020, 7:49 AM
mclow.lists added inline comments.
libcxx/include/any
382

Remember that we're not talking about an arbitrary allocator, but std::allocator
(It can be a user specialization, but it's still std::allocator)

See http://eel.is/c++draft/allocator.globals. They're always equal.

mclow.lists edited the summary of this revision. (Show Details)Jun 4 2020, 7:56 AM
mclow.lists marked an inline comment as done.Jun 4 2020, 8:43 AM
mclow.lists added inline comments.
libcxx/include/any
370

Sure. A user-specialization of std::allocator can do most anything, as long as it actually constructs the object as well.

Register it in a global registry, for example.

ldionne commandeered this revision.Sep 15 2020, 6:58 AM
ldionne added a reviewer: mclow.lists.
ldionne added a subscriber: ldionne.

I like this change. It's simple but I think it makes sense. I'll add tests and finish it up.

ldionne updated this revision to Diff 291928.Sep 15 2020, 7:52 AM

Add tests

Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptSep 15 2020, 7:52 AM
ldionne accepted this revision as: Restricted Project.Sep 15 2020, 7:53 AM

@tkoeppe Does this look like a suitable resolution to PR45099?

@tkoeppe Does this look like a suitable resolution to PR45099?

Without speaking for the dense implementationese, the direction looks good. Thanks!

@tkoeppe Does this look like a suitable resolution to PR45099?

Without speaking for the dense implementationese, the direction looks good. Thanks!

Basically this will always use std::allocator<T> to allocate construct destroy deallocate inside std::any. Shipping!

This revision was not accepted when it landed; it landed in state Needs Review.Sep 15 2020, 8:05 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.