These patches split std::allocator, std::get_temporary_buffer and std::auto_ptr
outside of <memory>. More patches splitting out the whole header are coming.
Details
- Reviewers
curdeius Mordante - Group Reviewers
Restricted Project - Commits
- rG6a1ac88fc19a: [libc++] Split std::get_temporary_buffer out of <memory>
rG0b439e4cc9db: [libc++] Split std::allocator out of <memory>
rG26beecfe470b: [libc++] Split auto_ptr out of <memory>
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
As usual, I oppose this kind of patch unless it includes some "benefit" (e.g., simplifying the dependency graph). Right now this is just complexifying the graph.
The benefit is that <memory> is gradually becoming smaller. Locally, where I have more patches applied, I've started discovering things like helpers that are utterly misplaced (e.g. a helper that lives in <memory> but has no business being there, since it's only used in e.g. <algorithm>). Overall, this is going to make it easier to navigate and understand the library.
libcxx/include/__memory/temporary_buffer.h | ||
---|---|---|
29 | Sidenote, why isn't it guarded with #if _LIBCPP_STD_VER <= 14 || defined(_LIBCPP_ENABLE_CXX17_REMOVED_xyz) like auto_ptr? | |
libcxx/include/memory | ||
685 | IMO there would be a "real" benefit of splitting those headers if their inclusion were guarded by #if _LIBCPP_STD_VER <= 14 || defined(_LIBCPP_ENABLE_CXX17_REMOVED_xyz). WDYT? By real, I mean more than just going down with the line count. |
libcxx/include/__memory/temporary_buffer.h | ||
---|---|---|
29 | I suspect this is simply an oversight, honestly. I think moving away from gigantic headers will make it easier to reason and spot this sort of stuff, too. Thinking twice, it may be that we're using get_temporary_buffer() to implement some algorithms so we can't actually remove it. Of course we could move to an internal name and stop providing the public name. I'm not changing any code whatsoever in this patch, just moving stuff around. | |
libcxx/include/memory | ||
685 | Yes, I fully agree, and in fact this is how I had done it originally. But I decided to stage it into multiple changes to avoid making any sort of code changes in the patches where I'm moving stuff around. Subtle mistakes are so easy to make. |
LGTM. I prefer having clearly cut headers. IMO it is better to have more than not enough. That's of course subjective.
BTW. Should I understand that we settled on __memory/* naming scheme?
libcxx/include/__memory/temporary_buffer.h | ||
---|---|---|
29 | Indeed, it's used in`<algorithm>`. |
It's "subjective", but then anything related to readability is subjective. I still think the benefits of such a simple change will be overwhelming in the long run -- I'm already starting to see a lot of structure in the library that I didn't see before, yet I've been working on it full time for 3 years.
BTW. Should I understand that we settled on __memory/* naming scheme?
For now it does the job. I'm not entirely satisfied, for example __memory/compressed_pair.h isn't great since __compressed_pair isn't strongly tied to <memory>. I think better might be __detail/compressed_pair.h for things that are not clearly tied to a specific public header. But it'll be easy to change that in the future if we want, and git handles renames pretty well, so there shouldn't be any churn generated from changing in the future.
One question other than that LGTM!
libcxx/include/__memory/allocator.h | ||
---|---|---|
11 | <__string> contains a duplicate of the synopsis of char_traits. These new headers don't have a synopsis. Is that intended or an oversight? |
libcxx/include/__memory/allocator.h | ||
---|---|---|
11 | I asked on Discord whether we wanted to keep the synopsis in the public header or split it up in the sub-headers, and we agreed to put the synopsis only in the public header. So I'll remove the synopsis from __string in a separate commit for consistency, thanks for the observation. |
libcxx/include/__memory/allocator.h | ||
---|---|---|
11 | Ok that's fine for me as well. I think consistency is the keyword. If we tend to do more smaller headers one synopsis increases the readability. |
<__string> contains a duplicate of the synopsis of char_traits. These new headers don't have a synopsis. Is that intended or an oversight?