This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Split allocator_traits and pointer_traits out of <memory>
ClosedPublic

Authored by ldionne on Dec 10 2020, 3:46 PM.

Details

Summary

In addition to making the code a lot easier to grasp by localizing many
helper functions to the only file where they are actually needed, this
will allow creating helper functions that depend on allocator_traits
outside of <memory>.

This is done as part of implementing array support in allocate_shared,
which requires non-trivial array initialization algorithms that would be
better to keep out of <memory> for sanity. It's also a first step towards
splitting up our monolithic headers into finer grained ones, which will
make it easier to reuse functionality across the library. For example,
it's just weird that we had to define addressof inside <type_traits>
to avoid circular dependencies -- instead it's better to implement those
in true helper headers.

Diff Detail

Event Timeline

ldionne created this revision.Dec 10 2020, 3:46 PM
ldionne requested review of this revision.Dec 10 2020, 3:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2020, 3:46 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
miscco added a subscriber: miscco.Dec 11 2020, 11:37 AM

I love hat you start breaking up the headers.

libcxx/include/__memory/allocator_traits.h
15

Should this also be __base.h to make clear that these are purely internal headers?

35

Maybe __pointer_type_impl ?

I have never seen imp as an abreviation. That said it is kind of fitting ;)

38

I _guess_ Dp stands for decayed type but I am not even sure

Could we give it a better name so that it is more obvious for new readers what it is?

libcxx/include/__memory/base.h
73

Missing comment here

I agree with your comments, however this is a split only, I'm keeping the code 100% the same to avoid mixing concerns. I actually do have a patch coming up that refactors allocator_traits, where I'll make sure to apply your comments.

libcxx/include/__memory/allocator_traits.h
15

I've put the headers in a directory that had __ in it to make it clear it was internal (and to avoid conflicting with user-provided directories as much as possible). I don't think it's necessary to have the file contain __ too IMO.

tschuett added a subscriber: tschuett.EditedDec 11 2020, 11:42 AM

Does __memory/* gets its own entry in the module.modulemap?

Does __memory/* gets its own entry in the module.modulemap?

I don't think that's necessary since it's an implementation detail -- kinda like the __support/ headers are not mentioned in the modulemap. Do you agree?

tschuett added a comment.EditedDec 11 2020, 1:24 PM

It is not necessary, but modules are faster to parse than headers.

-> I don't care.

Clang supports "private modules".
https://clang.llvm.org/docs/Modules.html#private-module-map-files

Somehow the private module doesn't work because module.modulemap contains two modules (std_config and std).

It is not necessary, but modules are faster to parse than headers.

-> I don't care.

Clang supports "private modules".
https://clang.llvm.org/docs/Modules.html#private-module-map-files

Somehow the private module doesn't work because module.modulemap contains two modules (std_config and std).

Hmm, I see. Ok, well thanks for trying anyway. I guess this is going in as-is then.

ldionne updated this revision to Diff 311660.Dec 14 2020, 11:32 AM

Rebase onto master

ldionne accepted this revision.Dec 14 2020, 11:34 AM

I'll ship this once CI passes.

This revision is now accepted and ready to land.Dec 14 2020, 11:34 AM
Quuxplusone added inline comments.
libcxx/include/__memory/allocator_traits.h
15

FWIW, I just saw this today, and I wish the level of directory structure weren't there; I'd rather have seen <__memory_base>, <__memory_pointer_traits>, <__memory_allocator_traits>. I don't even know what's in <__memory/utilities.h>, so I probably wouldn't have split it out — just combine it into <__memory_base> if it's needed by lots of things, and otherwise leave it in <memory>.

libcxx/include/__memory/allocator_traits.h
15

Worse, I have just noticed that this moves addressof out of <type_traits> and into base.h, which depends on <type_traits>; so my <type_traits> can no longer use addressof to implement swap for trivially relocatable types. In my branch, I'm moving addressof back into <type_traits>.

I do have some vague ideas re a new <__type_traits_base>, to contain declval and forward and move and addressof and swap and the helpers like _EnableIf and whatever type-traits seem most used (e.g. is_*_constructible, is_integral, is_signed). But, I don't think that's really worth pursuing, because libc++'s current philosophy is "<type_traits> goes at the root"; there's no point in trying to make other headers not include <type_traits> because everyone is going to include <type_traits> at some point anyway.

I agree with trying to extract a <__memory_base>. Also, in C++20 we'll need an <__algorithm_base> to avoid dragging all the Ranges stuff into <vector> and <string>.