Moves:
- std::move, std::forward, std::declval, and std::swap into __utility/${FUNCTION_NAME}.
- std::swap_ranges and std::iter_swap into __algorithm/${FUNCTION_NAME}
Paths
| Differential D103734
[libcxx][modularisation] moves <utility> content out of <type_traits> ClosedPublic Authored by cjdb on Jun 4 2021, 7:53 PM.
Details
Summary Moves:
Diff Detail
Event TimelineComment Actions I like this direction! I assume the patch has no changes in the moved code.
Comment Actions I'd like to better understand the motivation for this patch - are you working towards something specific? Generally LGTM.
This revision now requires changes to proceed.Jun 8 2021, 6:49 AM Comment Actions transitively puts std::swap definition back into <type_traits> and adds a TODO so it can be removed in a future patch dedicated to header minimisation Comment Actions
I'm breaking down <functional> into appropriate units so that __functional and friends can disappear (they don't play well with the module map and are really clunky to begin with). So, I guess the final destination of this patch is to see <utility> and <functional> broken down. <type_traits> will likely get taken along for the ride too, but I have no (immediate) intention of looking at that header as a whole.
Comment Actions
I made one minor change to swap, where I replaced the return type (which was obfuscated by ifdefs) with an alias so the compiler selection doesn't screw up readability. It's still non-functional. Comment Actions
Why? Can't you split <functional> without tackling down <utility>? Note: Generally speaking, I'm very happy to split up headers into smaller headers that make sense (I've been fighting hard for that since the beginning). But I'm struggling to see the structure in this patch. It just looks to me as though random bits are being split into headers, and also I'm not sure about the granularity. In particular, splitting up type traits this way (one trait per header, but not always) does not appear like an improvement to me. If you're trying to split up <functional>, is it possible to do it without touching other headers? If not, let's figure out a way forward. Also, do you really have to touch anything in <type_traits> except stuff that would clearly belong elsewhere (like iter_swap and swap_ranges, which are great examples of libc++'s failure to sanely split things with monolithic headers)? This revision now requires changes to proceed.Jun 8 2021, 12:57 PM Comment Actions
Sadly, no. Assuming I've properly split <functional>, I get a circular dep between the two. IIRC it's specifically std::pair that's causing me grief right now, but that depends on a fair bit in <utility>, so I decided that would be a better starting point.
I make no defence for how I've split <type_traits>. I'd prefer one trait per header, but I think you had other ideas and so I kind of went "we can properly fix this when someone has the time/courage to turn to <type_traits>".
See above. And below. It's certainly not possible to avoid touching <utility>, because that's where unary_function currently lives.
I've moved out the bare minimum of <type_traits> in this patch to facilitate the things I've put into self-contained headers. std::move, std::forward, and std::swap aren't supposed to be in <type_traits>, but the monolithic header model kinda forced us to do it this way. Comment Actions Recording offline discussion with Chris about how to move this forward:
Now, <type_traits> should not depend on <utility> anymore (to validate). So then:
This should allow us to not move anything that belongs into <type_traits> (according to the standard) outside of <type_traits>. Comment Actions @ldionne wrote:
Yes please.
This likely won't work; but also, the historical libc++ approach here is to define all of the swap, iter_swap, swap_ranges algorithms directly in <type_traits> — they operate as a cohesive unit, so it makes sense to keep them together; and they are fundamental to a lot of libc++, so it makes sense to keep them in the "root-of-the-graph" header <type_traits> rather than trying to lift them up into some less fundamental header like <utility>. I strongly recommend keeping these three fundamental swap-related algorithms together. If you want to pull them out into something like __utility/swap.h or __type_traits/swap.h, that seems fine; but I would not recommend attempting to split swap apart from swap_ranges or even iter_swap.
I would say, great examples of WG21's failure to understand cyclic dependencies. libc++ is doing the best it can. :) cjdb retitled this revision from [libcxx] moves <utility> content out of <type_traits> and into their own headers to [libcxx][modularisation] moves <utility> content out of <type_traits>. Comment Actions
cjdb added a child revision: D104002: [libcxx][modularisation] splits `<utility>` into self-contained headers.Jun 9 2021, 6:12 PM ldionne added inline comments.
This revision now requires changes to proceed.Jun 21 2021, 8:07 AM
cjdb added a parent revision: D104738: [libcxx][NFC] prepares `<type_traits>` for moving out forward and swap.Jun 22 2021, 12:45 PM Comment Actions LGTM w/ comment.
This revision is now accepted and ready to land.Jun 22 2021, 1:40 PM
This revision was landed with ongoing or failed builds.Jun 24 2021, 11:02 AM Closed by commit rG6adbc83ee9e4: [libcxx][modularisation] moves <utility> content out of <type_traits> (authored by cjdb). · Explain Why This revision was automatically updated to reflect the committed changes. cjdb removed a child revision: D104002: [libcxx][modularisation] splits `<utility>` into self-contained headers.Jun 24 2021, 11:11 AM cjdb removed a child revision: D104002: [libcxx][modularisation] splits `<utility>` into self-contained headers.Jun 24 2021, 11:17 AM cjdb removed a child revision: D104002: [libcxx][modularisation] splits `<utility>` into self-contained headers.Jun 24 2021, 1:06 PM
Revision Contents
Diff 354306 libcxx/include/CMakeLists.txt
libcxx/include/__algorithm/inplace_merge.h
libcxx/include/__algorithm/iter_swap.h
libcxx/include/__algorithm/move.h
libcxx/include/__algorithm/next_permutation.h
libcxx/include/__algorithm/nth_element.h
libcxx/include/__algorithm/partial_sort.h
libcxx/include/__algorithm/partition.h
libcxx/include/__algorithm/pop_heap.h
libcxx/include/__algorithm/prev_permutation.h
libcxx/include/__algorithm/push_heap.h
libcxx/include/__algorithm/remove.h
libcxx/include/__algorithm/reverse.h
libcxx/include/__algorithm/rotate.h
libcxx/include/__algorithm/shift_right.h
libcxx/include/__algorithm/shuffle.h
libcxx/include/__algorithm/sift_down.h
libcxx/include/__algorithm/sort.h
libcxx/include/__algorithm/stable_partition.h
libcxx/include/__algorithm/stable_sort.h
libcxx/include/__algorithm/swap_ranges.h
libcxx/include/__algorithm/unique.h
libcxx/include/__iterator/concepts.h
libcxx/include/__iterator/iter_move.h
libcxx/include/__memory/allocator.h
libcxx/include/__memory/allocator_traits.h
libcxx/include/__memory/compressed_pair.h
libcxx/include/__memory/construct_at.h
libcxx/include/__memory/shared_ptr.h
libcxx/include/__memory/unique_ptr.h
libcxx/include/__ranges/access.h
libcxx/include/__ranges/all.h
libcxx/include/__ranges/data.h
libcxx/include/__ranges/empty.h
libcxx/include/__ranges/size.h
libcxx/include/__split_buffer
libcxx/include/__tree
libcxx/include/__utility/__decay_copy.h
libcxx/include/__utility/declval.h
libcxx/include/__utility/forward.h
libcxx/include/__utility/move.h
libcxx/include/__utility/swap.h
libcxx/include/algorithm
libcxx/include/any
libcxx/include/deque
libcxx/include/exception
libcxx/include/experimental/iterator
libcxx/include/filesystem
libcxx/include/forward_list
libcxx/include/functional
libcxx/include/future
libcxx/include/istream
libcxx/include/iterator
libcxx/include/list
libcxx/include/map
libcxx/include/module.modulemap
libcxx/include/mutex
libcxx/include/queue
libcxx/include/scoped_allocator
libcxx/include/set
libcxx/include/stack
libcxx/include/thread
libcxx/include/tuple
libcxx/include/type_traits
libcxx/include/unordered_map
libcxx/include/unordered_set
libcxx/include/utility
libcxx/include/variant
libcxx/include/vector
libcxx/test/std/utilities/utility/forward/forward.fail.cpp
libcxx/test/std/utilities/utility/utility.swap/swap.pass.cpp
libcxx/test/std/utilities/utility/utility.swap/swap_array.pass.cpp
libcxx/test/support/poisoned_hash_helper.h
|
This forward declaration isn't using the __swap_result_t you factored out. I think you should include __utility/swap.h here.