This is an archive of the discontinued LLVM Phabricator instance.

[libc++] NFCI: Refactor allocator_traits
ClosedPublic

Authored by ldionne on Jan 12 2021, 12:33 PM.

Details

Reviewers
zoecarver
Group Reviewers
Restricted Project
Commits
rG2cb4a96a99e8: [libc++] NFCI: Refactor allocator_traits
Summary

The implementation had a lot of boilerplate and was more complicated than
necessary. This NFC refactoring introduces a few macros to reduce code
duplication, and uses a consistent style and formatting for the whole file.

Diff Detail

Event Timeline

ldionne created this revision.Jan 12 2021, 12:33 PM
ldionne requested review of this revision.Jan 12 2021, 12:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2021, 12:33 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Thanks for the cleanup, this is much improved!

libcxx/include/__memory/allocator_traits.h
159

If this is the only use of _LIBCPP_ALLOCATOR_TRAITS_HAS_XXX_TEMPLATE, is it better to just implement it here (and get rid of the macro)?

190

Let's put these at the bottom of the file. Otherwise, if someone tries to use them later on it might create a hard-to-fix error.

244

Could this be __allocator_traits_rebind_t?

272

Why the has_allocate_hint? Is that just for the deprecation warnings? Is it worth just taking the deprecation warnings so we could have just one implementation? For compile-time if nothing else. Those SFINAE checks are slow.

295

I don't really understand why we have two implementations here. This isn't a constexpr context, so they both do the same thing.

ldionne updated this revision to Diff 317397.Jan 18 2021, 11:38 AM
ldionne marked 4 inline comments as done.

Address Zoe's comments. Note that this is meant to be a NFC commit, so I don't
think we should incorporate additional changes beyond the refactorign to those
changes.

libcxx/include/__memory/allocator_traits.h
272

Do you mean that you'd prefer doing the check for whether a.allocate(n, hint) is supported inline in the template declaration?

If you're asking why we call allocate with a hint at all, it's because we must. If a user-defined allocator does something special when called with a hint, we need to maintain that.

295

construct_at isn't provided before C++20.

zoecarver added inline comments.Jan 18 2021, 12:03 PM
libcxx/include/__memory/allocator_traits.h
272

Oops, I didn't see that we're only passing the hint argument in one of these. Never mind then :)

295

I was thinking the opposite: always construct with placement new. Is there a way (that isn't UB) for a user to know the difference?

(This is also a super small thing, no need to address it, I know this is a bit out-of-scope.)

zoecarver accepted this revision.Jan 18 2021, 12:04 PM

Thanks again for the cleanup!

ldionne accepted this revision as: Restricted Project.Jan 18 2021, 2:36 PM
ldionne marked an inline comment as done.

Thanks for the review Zoe!

libcxx/include/__memory/allocator_traits.h
295

The issue is that placement-new doesn't work inside constexpr, but construct_at does. This function is marked as constexpr in C++20 and above (_LIBCPP_CONSTEXPR_AFTER_CXX17).

This revision is now accepted and ready to land.Jan 18 2021, 2:36 PM
This revision was automatically updated to reflect the committed changes.
ldionne marked an inline comment as done.