Page MenuHomePhabricator

[libc++] Move the definition of aligned allocation helpers outside of <new>
ClosedPublic

Authored by ldionne on Dec 2 2022, 1:52 PM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Restricted Project
Commits
rG36080434a885: [libc++] Move the definition of aligned allocation helpers outside of <new>
Summary

They are not needed in <new> -- in fact they are only needed in .cpp files.
Getting those out of the way makes the headers smaller and also makes it
easier to use the library on platforms where aligned allocation is not
available.

Diff Detail

Event Timeline

ldionne created this revision.Dec 2 2022, 1:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2022, 1:52 PM
ldionne requested review of this revision.Dec 2 2022, 1:52 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 2 2022, 1:52 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne updated this revision to Diff 480095.Dec 5 2022, 6:59 AM

Try to fix CI

Mordante added inline comments.
libcxx/include/__memory/aligned_alloc.h
11

Why is the header not included by __memory? When intended it would be good to document why it's the case.
Then the guarded include in the .cpp files can be removed too.

20

Shouldn't this code be guarded by _LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION?

ldionne updated this revision to Diff 492238.Wed, Jan 25, 1:03 PM
ldionne marked 2 inline comments as done.

Rebase and address some comments

libcxx/include/__memory/aligned_alloc.h
11

Like we do for many other implementation-detail headers, we don't include them in the main public header unless they provide something that is part of the public. We actually have the opposite policy (to include everything, as you mention), but we do it very inconsistently, sometimes because it's literally impossible to do otherwise. Hence, I think we should shift the policy towards including only the things that are necessary for the public API.

ldionne updated this revision to Diff 492309.Wed, Jan 25, 6:43 PM

Try to fix CI

ldionne accepted this revision.Thu, Jan 26, 11:40 AM

Both CI failures are flakes.

This revision is now accepted and ready to land.Thu, Jan 26, 11:40 AM
This revision was landed with ongoing or failed builds.Thu, Jan 26, 11:41 AM
This revision was automatically updated to reflect the committed changes.
Mordante added inline comments.Fri, Jan 27, 9:22 AM
libcxx/include/__memory/aligned_alloc.h
11

I think it would be good to document our policy. I sometimes get the impression different developers have different ideas what the policies are.