Page MenuHomePhabricator

[libc++] Consolidate swap, swap_ranges, and iter_swap in <type_traits>. NFC.
ClosedPublic

Authored by Quuxplusone on Jul 24 2019, 7:37 PM.

Details

Summary

Right now in master, <type_traits> contains the implementation of swap(T&, T&), a forward-declaration of swap(T(&)[N], T(&)[N]), and the implementation of iter_swap in terms of ADL swap. Then, <utility> (which includes <type_traits>) contains the implementation of swap(T(&)[N], T(&)[N]) in terms of std::swap_ranges, plus the implementation of swap_ranges.

This means that when the user's program does #include <type_traits>, they get a declaration but no definition for swap(T(&)[N], T(&)[N]) — not technically wrong, not a bug, but mildly surprising. Also, this seems to mean that changing the enable_if business on swap(T(&)[N], T(&)[N]) would count as an ABI break (if only for user programs that fail to include-what-they-use and thus technically are non-conforming IIUC).

Most importantly, it means that when I want to modify swap (say, to add another overload specifically for trivially relocatable types), there are two headers to modify instead of just one. :)

After this patch, <type_traits> contains the implementation of swap(T&, T&); the implementation of swap(T(&)[N], T(&)[N]) in terms of std::swap_ranges; the implementation of swap_ranges; and the implementation of iter_swap in terms of ADL swap. It's a one-stop swap shop! Then <utility> simply includes <type_traits>, as before.

I don't intend this patch to have any observable effect for users.

Diff Detail

Repository
rCXX libc++

Event Timeline

Quuxplusone created this revision.Jul 24 2019, 7:37 PM
ldionne accepted this revision.Jul 25 2019, 6:59 AM

I don't mind this change.

However, I really dislike the general organization (or rather, the lack thereof) in libc++. It's weird to be defining swap and swap_ranges inside <type_traits>, when swap_ranges should be part of <algorithm> and swap should be in <utility>.

This revision is now accepted and ready to land.Jul 25 2019, 6:59 AM

I really dislike the general organization (or rather, the lack thereof) in libc++. It's weird to be defining swap and swap_ranges inside <type_traits>, when swap_ranges should be part of <algorithm> and swap should be in <utility>.

I'd call that a lack of organization in the paper standard, not in the implementation! According to N4810,

  • <concepts> defines Swappable in terms of ranges::swap, and defines ranges::swap in terms of ADL swap
  • <utility> defines swap(T&, T&), and defines swap(T (&)[N], T (&)[N])
  • <type_traits> defines is_swappable in terms of std::swap and ADL swap
  • <iterator> defines ranges::iter_swap in terms of ranges::swap and ADL iter_swap(!!)
  • <algorithm> defines std::iter_swap in terms of std::swap and (arguably) ADL swap; defines std::swap_ranges in terms of std::swap and (arguably) ADL swap; defines ranges::swap_ranges in terms of ranges::iter_swap
  • <concepts> includes nothing
  • <utility> includes only <initializer_list>
  • <type_traits> includes nothing, despite requiring std::swap out of <utility>
  • <iterator> includes only <concepts>
  • <algorithm> includes only <initializer_list>, despite requiring std::swap out of <utility> and ranges::iter_swap out of <iterator>

Taking that spaghetti literally would result in a maintenance nightmare.

I guess this has been languishing. @ldionne, thoughts on maybe landing this patch?

Rebased on master; the patch still applied cleanly. @ldionne ping?

zoecarver accepted this revision.Wed, Sep 11, 9:37 AM

zoecarver accepted this revision

@zoecarver is not an approver for libc++.

Sorry, I simply meant "this looks good to me." Next time, I will just comment that :)

I assume @ldionne _is_ an approver for libc++ at this point? And either way, I haven't got commit access, which is why I continue to ping Louis despite his earlier marking this patch as accepted.

mclow.lists accepted this revision.Wed, Sep 11, 10:15 AM

With the comment change I suggested, I am fine with this.

include/utility
253

I would just say:

// swap_ranges is defined in <type_traits>`

// swap is defined in <type_traits>

and leave it at that.

zoecarver closed this revision.Wed, Sep 11, 10:38 AM

Thanks for the patch @Quuxplusone. Committed as rL371639.

Sorry -- yes, I am fine with this patch. I just forgot to commit it.