This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][modularisation] moves <utility> content out of <type_traits>
ClosedPublic

Authored by cjdb on Jun 4 2021, 7:53 PM.

Details

Summary

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}

Diff Detail

Event Timeline

cjdb created this revision.Jun 4 2021, 7:53 PM
cjdb requested review of this revision.Jun 4 2021, 7:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2021, 7:53 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb updated this revision to Diff 350011.Jun 4 2021, 8:04 PM

fixes circular dep

Mordante accepted this revision as: Mordante.Jun 6 2021, 2:00 AM

I like this direction! I assume the patch has no changes in the moved code.
LGTM modulo one minor nit.

libcxx/include/type_traits
425

Can you add the reason for the removal in the comment?

ldionne requested changes to this revision.Jun 8 2021, 6:49 AM

I'd like to better understand the motivation for this patch - are you working towards something specific? Generally LGTM.

libcxx/include/__decay_copy.h
1 ↗(On Diff #350011)

Why is this at the top level?

libcxx/test/std/utilities/utility/utility.swap/swap_array.pass.cpp
17

This is using is_same, so it needs <type_traits>.

This revision now requires changes to proceed.Jun 8 2021, 6:49 AM
cjdb updated this revision to Diff 350673.Jun 8 2021, 11:28 AM

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

cjdb marked 2 inline comments as done.Jun 8 2021, 11:29 AM

I'd like to better understand the motivation for this patch - are you working towards something specific? Generally LGTM.

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).
<utility> needs to be broken down first, in order to achieve that.

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.

libcxx/include/__decay_copy.h
1 ↗(On Diff #350011)

I didn't really know where to put it. It's in __type_traits now, but I should probably move it to __utility before merging.

cjdb added a comment.EditedJun 8 2021, 11:35 AM

I like this direction! I assume the patch has no changes in the moved code.
LGTM modulo one minor nit.

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.

ldionne requested changes to this revision.Jun 8 2021, 12:57 PM

<utility> needs to be broken down first, in order to achieve that.

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
cjdb added a comment.Jun 8 2021, 1:20 PM

<utility> needs to be broken down first, in order to achieve that.

Why? Can't you split <functional> without tackling down <utility>?

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.

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.

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>".

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.

See above. And below. It's certainly not possible to avoid touching <utility>, because that's where unary_function currently lives.

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)?

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.

Recording offline discussion with Chris about how to move this forward:

  1. Use static_cast<T&&> instead of std::forward in <type_traits> (this should only impact the implementation of __invoke)
  2. Forward declare std::swap in <type_traits> just to implement the is_XXX_swappable_with traits

Now, <type_traits> should not depend on <utility> anymore (to validate). So then:

  1. Move things that should be in <utility> but that are defined in <type_traits> out of <type_traits>

This should allow us to not move anything that belongs into <type_traits> (according to the standard) outside of <type_traits>.

@ldionne wrote:

Recording offline discussion with Chris about how to move this forward:

Use static_cast<T&&> instead of std::forward in <type_traits> (this should only impact the implementation of __invoke)

Yes please.

Forward declare std::swap in <type_traits> just to implement the is_XXX_swappable_with traits

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.

great examples of libc++'s failure to sanely split things with monolithic headers

I would say, great examples of WG21's failure to understand cyclic dependencies. libc++ is doing the best it can. :)

cjdb updated this revision to Diff 350744.Jun 8 2021, 5:12 PM
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>.
cjdb edited the summary of this revision. (Show Details)
  • reverts moving type traits out of <type_traits>
  • removes all traces of <utility> from <type_traits>
cjdb updated this revision to Diff 353019.Jun 18 2021, 8:56 AM

rebases to get new <algorithm> split

ldionne requested changes to this revision.Jun 21 2021, 8:07 AM
ldionne added inline comments.
libcxx/include/__utility/swap.h
34

Could you make this change as a separate NFC commit? I really want those modularization changes to contain no change whatsoever to the code itself. By the way, did you make any other change to the code except the std::forward => static_cast<T&&> transformation?

libcxx/include/type_traits
3869

I'd like to make this change as a separate NFC change, for the reason stated above.

libcxx/test/support/poisoned_hash_helper.h
12

We should never be including implementation detail headers from the test suite. If we can't include <utility> right now, we should do the pre-requisite task before landing this patch.

This revision now requires changes to proceed.Jun 21 2021, 8:07 AM
ldionne added inline comments.Jun 21 2021, 8:50 AM
libcxx/test/support/poisoned_hash_helper.h
12

Nevermind this comment, I just saw https://reviews.llvm.org/D104002.

cjdb updated this revision to Diff 353752.Jun 22 2021, 12:45 PM

splits NFC changes out

ldionne accepted this revision.Jun 22 2021, 1:40 PM

LGTM w/ comment.

libcxx/include/__algorithm/swap_ranges.h
28

This forward declaration isn't using the __swap_result_t you factored out. I think you should include __utility/swap.h here.

This revision is now accepted and ready to land.Jun 22 2021, 1:40 PM
libcxx/include/__algorithm/swap_ranges.h
28

Since std::swap is implemented in terms of std::swap_ranges, which itself is implemented in terms of std::swap, I think both of them should go into the same helper header. In main right now, that header is <type_traits>. I don't object to the idea of pulling them out into a single helper header, e.g. __utility/swap.h; but they ought to stay together, because cyclic dependency.

cjdb updated this revision to Diff 353824.Jun 22 2021, 4:55 PM

rebases to fix modules CI

This revision was landed with ongoing or failed builds.Jun 24 2021, 11:02 AM
This revision was automatically updated to reflect the committed changes.