[libc++] Implements ranges::enable_borrowed_range

[Mordante]

Summary

This is the initial patch to implement ranges in libc++.

Implements parts of:

• P0896R4 One Ranges Proposal
• P1870 forwarding-range is too subtle
• LWG3379 in several library names is misleading

[Mordante]

Rebased to update to main and a friendly ping.

[cjdb]

Overall, LGTM, but I'm wondering if we should hold off on merging till we've got consensus on how detail headers are going to be handled.
This is only because I'm seeing __ranges_enable_helpers.

libcxx/include/ranges

16

It's transitively included by <iterator>.

libcxx/test/std/ranges/range.range/enable_borrowed_range.compile.pass.cpp

2

Please change to enable_borrowed_range.pass.cpp (we've agreed in other patches to move away from .compile). Same with other files (this is just the first I noticed).

[Quuxplusone]

libcxx/include/module.modulemap

414

We don't do this line for any other header, e.g. __functional_base, so I don't think it should be necessary here.

[Mordante]

Addresses review comments, rebased on main.

[Mordante]

libcxx/include/module.modulemap

414

I looked at __string, but that seems to be the exception. So I'll remove this one.

libcxx/include/ranges

16

Agreed, but this mandated by the standard. Since the overhead is minimal I prefer to follow the standard. If in a future revision <initializer_list> no longer requires <compare> we don't silently break our implementation.

libcxx/test/std/ranges/range.range/enable_borrowed_range.compile.pass.cpp

2

In the past had requests to rename test to use .compile. I asked for a policy on its usage on Discord, depending on the answer there I'll keep this name or rename the file.

[Mordante]

libcxx/test/std/ranges/range.range/enable_borrowed_range.compile.pass.cpp

2

As discussed on Discord we can keep compile only tests .compile. To future-proof them they shouldn't contain a main. So I'll update the .compile tests to have no main.

[Mordante]

Remove main() from the compile time tests.
Rebased on main.

[cjdb]

libcxx/include/__ranges_enable_helpers

1 ↗

(On Diff #335849)

Please change the path to __ranges/enable_helpers.h and move the synopsis to <ranges>.

38 ↗

(On Diff #335849)

We've got a _LIBCPP_HAS_NO_RANGES macro now that guards on C++20 now.

[zoecarver]

libcxx/include/__ranges_enable_helpers

10 ↗

(On Diff #335849)

I don't think we ever use triple underscore. This makes it hard to read. I think single underscore here is sufficient.

libcxx/include/module.modulemap

414

Hmm, my vote would be to export it as well. But I don't feel strongly. Do we have a policy on this @ldionne @EricWF?

libcxx/include/ranges

39

Same as the other comment, you can use _LIBCPP_HAS_NO_RANGES (in fact, let's guard the whole header on that).

41

Nit: I don't think it makes sense to have an empty namespace here.

[Quuxplusone]

libcxx/include/__ranges_enable_helpers

10 ↗

(On Diff #335849)

Actually this is libc++ style, and has the benefit of being machine-generable (the header-inclusion test generator actually relies on their machine-generability, but only for public headers, not for these internal headers):

$ git grep '#define.*___' __*
__availability:#define _LIBCPP___AVAILABILITY
__bit_reference:#define _LIBCPP___BIT_REFERENCE
__bits:#define _LIBCPP___BITS
__errc:#define _LIBCPP___ERRC
__locale:#define _LIBCPP___LOCALE
__memory/allocator.h:#define _LIBCPP___MEMORY_ALLOCATOR_H
__memory/allocator_traits.h:#define _LIBCPP___MEMORY_ALLOCATOR_TRAITS_H
__memory/auto_ptr.h:#define _LIBCPP___MEMORY_AUTO_PTR_H
__memory/base.h:#define _LIBCPP___MEMORY_BASE_H
__memory/pointer_traits.h:#define _LIBCPP___MEMORY_POINTER_TRAITS_H
__memory/temporary_buffer.h:#define _LIBCPP___MEMORY_TEMPORARY_BUFFER_H
__memory/utilities.h:#define _LIBCPP___MEMORY_UTILITIES_H
__mutex_base:#define _LIBCPP___MUTEX_BASE
__node_handle:#define _LIBCPP___NODE_HANDLE
__sso_allocator:#define _LIBCPP___SSO_ALLOCATOR
__std_stream:#define _LIBCPP___STD_STREAM
__string:#define _LIBCPP___STRING
__tree:#define _LIBCPP___TREE
__tuple:#define _LIBCPP___TUPLE

The current exceptions-to-the-rule are

__config:#define _LIBCPP_CONFIG
__debug:#define _LIBCPP_DEBUG_H
__functional_03:#define _LIBCPP_FUNCTIONAL_03
__functional_base:#define _LIBCPP_FUNCTIONAL_BASE
__functional_base_03:#define _LIBCPP_FUNCTIONAL_BASE_03
__hash_table:#define _LIBCPP__HASH_TABLE
__nullptr:#define _LIBCPP_NULLPTR
__split_buffer:#define _LIBCPP_SPLIT_BUFFER

That said, this header name is a bit unwieldy, and vague to boot. <__enable_borrowed_range> would be strictly better. Plus, I imagine @ldionne will push for <__ranges/enable_borrowed_range.h> to match the new <__memory/*.h> regime.

If there's some reason we might want more in this header than just enable_borrowed_range, I think it's important for someone to speak up and say so now. The only other "enable helper" I'm aware of is disable_sized_range, and that can go loose in <ranges> because (unlike enable_borrowed_range) disable_sized_range is never used by anything in the library.

libcxx/test/std/containers/views/enable_borrowed_range.compile.pass.cpp

30–31

Same thing tested twice in a row. What was meant here? Ditto twice in the lines below.

36

Again, I recommend using something other than 1 as your "regular ordinary span" type. I suggest 10.

libcxx/test/std/language.support/support.limits/support.limits.general/ranges.version.pass.cpp

11

When adding a new header, I strongly recommend git grep-ing for the names of some other recently added headers, e.g. <charconv> and <barrier> and <concepts>, and seeing where they're used, both in util/ and in test/.

You'll need to modify some of the generators in util/, notably the header-inclusion-test generator.

I can't think of anything else you missed off the top of my head, but grepping will always tell you.

libcxx/test/std/ranges/range.range/enable_borrowed_range.compile.pass.cpp

43

I recommend using something like std::array<int, 10> as your "regular ordinary std::array" example (in general, not limited to this specific test). array<int, 1> seems like it might get some special cases, especially in Ranges, where there's a lot of obscure logic around the nebulous distinction between "O(1)" and "O(n)".

[Quuxplusone]

libcxx/include/module.modulemap

414

FYI @zoecarver, this comment has moved since I made it. We shouldn't export __ranges_enable_borrowed_range or whatever we're calling it; but of course we should export ranges! :)

[Mordante]

libcxx/include/__ranges_enable_helpers

10 ↗

(On Diff #335849)

When I originally wrote this header we weren't as keen on splitting out our headers. I originally planned to add more small helpers to the header. IIRC disable_sized_range was on the list. But I agree it would be good avoid tech debt and change the header to our current style. I used __string as example which uses triple underscore. I don't have a strong opinion on what we use as long as it's consistent.

38 ↗

(On Diff #335849)

Thanks for the information, will adjust this code.

libcxx/include/ranges

39

I'll switch to the new guard.
I disagree on guarding the entire header. This is they normal way how we do things and I love consistency.
In fact guarding the entire header won't work; we need to include <__config> to define _LIBCPP_HAS_NO_RANGES.

libcxx/test/std/containers/views/enable_borrowed_range.compile.pass.cpp

30–31

It seems I wanted to test volatile but made a typical classical copy-paste error.

41

Dito.

[zoecarver]

Please fix the remaining few nits and land this! Thanks @Mordante! (You can accept on behalf of libc++ yourself.)

libcxx/include/__ranges_enable_helpers

10 ↗

(On Diff #335849)

Good to know. In that case I'm fine with triple underscore, or whatever's appropriate for the header name we land on.

[ldionne]

AFAICT, the only things are:

1. Rename the header to __ranges/borrowed_range.h or similar, with the intent of having both the concept and the helper in it.
2. Move the synopsis to <ranges>.

If I missed something else from another reviewer, please do it too, and then ship-it. No need for another round of reviews.

libcxx/include/__ranges_enable_helpers

10 ↗

(On Diff #335849)

I also have no opinion on the triple underscore story - I just did it that way cause it was consistent with the name of the files, but I agree triple underscores are not super pretty. 🤷🏼‍♂️

I think we should name this __ranges/borrowed_range.h and define both the concept (in the future) and this helper in it. As an example of what kind of granularity I think is good, I don't think it makes sense to have separate headers for std::enable_borrowed_range and the std::borrowed_range concept, not sure whether that was the intent in the long term or not.

libcxx/include/string_view

658

TBH, I haven't thought yet about whether we want _LIBCPP_INLINE_VISIBILITY on such inline variables. I guess it's fine if the compilers don't complain (I would have expected GCC to complain cause it expands to __always_inline__ on it).

[ldionne]

In D90999#2692573, @ldionne wrote:

AFAICT, the only things are:

1. Rename the header to __ranges/borrowed_range.h or similar, with the intent of having both the concept and the helper in it.

Forget about this. I just spoke to Arthur and he explained that the very purpose of splitting the customization point into its own header was to avoid dragging all of <ranges> in the other headers. That makes total sense, I had missed that. So then I think the name of the header should just reflect what's in it, i.e. enable_borrowed_range.h or the like.

Still LGTM with that naming sorted out.

[Mordante]

Thanks for all the reviews. I'll give it one more CI run to see whether the renaming didn't break things. Once that passes I'll land this patch.

libcxx/include/__ranges_enable_helpers

38 ↗

(On Diff #335849)

Note I still keep the _LIBCPP_STD_VER > 17 part, just like we did in <concepts>.

libcxx/include/string_view

658

This patch passed our CI so it should work with GCC. Since I still have quite some modifications to make I already wanted to give it one more CI run before landing.

libcxx/test/std/language.support/support.limits/support.limits.general/ranges.version.pass.cpp

11

IMO if something is needed for a new header it should be documented in https://libcxx.llvm.org/docs/Contributing.html#adding-a-new-header-todo.
Thanks for catching the missing python utils/generate_header_inclusion_tests.py;. That one wasn't done since that script wasn't available when I wrote this patch.

libcxx/test/std/ranges/range.range/enable_borrowed_range.compile.pass.cpp

43

Good suggestion, thanks!

[Mordante]

Addressed all review comments.
Rebased to main.

[Mordante]

Remove __ranges/enable_borrowed_range.h from include/module.modulemap.
This seems to break the build. The recently added headers in __memory
also aren't in this file so this seems not to be required.

[Mordante]

Disable ranges on Apple since it requires concepts.

[Mordante]

Disable another test for Apple.

[Quuxplusone]

LGTM % comments. I suspect we will end up removing _LIBCPP_INLINE_VISIBILITY in the place it's commented-on, but that can be done in post.

libcxx/include/span

525

Nit: I'd prefer not to see _VSTD:: here.
Could someone (presumably @ldionne) comment on whether _LIBCPP_INLINE_VISIBILITY is a good idea here? My intuitive impression is "no, it shouldn't be here"; compare how we did it in <numbers>. (But those are partial specializations expressed via C++20 constraints, whereas this is a partial specialization expressed in the old-fashioned way. I don't know if that makes a subtle difference.)

(Note that _LIBCPP_INLINE_VAR is also a thing, but I think we don't want it here (because this is post-C++14-only) and my grepping didn't turn up any more precedents for what we're doing here with partial specialization.)

libcxx/test/std/containers/views/enable_borrowed_range.compile.pass.cpp

47–53

This test is overly complicated. Please replace the whole thing with these five lines, which test everything you've got now plus the fact that cvref-qualification matters to the specialization plus will give better error messages in the extremely unlikely case that this test ever fails.

void test() {
    static_assert(std::ranges::enable_borrowed_range<std::span<int, 0>>);
    static_assert(std::ranges::enable_borrowed_range<std::span<int, 42>>);
    static_assert(std::ranges::enable_borrowed_range<std::span<int, std::dynamic_extent>>);
    static_assert(!std::ranges::enable_borrowed_range<std::span<int, 42>&>);
    static_assert(!std::ranges::enable_borrowed_range<std::span<int, 42> const>);
}

[Mordante]

libcxx/include/span

525

@ldionne also had doubts, https://reviews.llvm.org/D90999#inline-947749. If we don't do that in <numbers> then I'll remove them before committing. The same for the _VSTD::.

[Mordante]

Addresses the final review comments.
Giving it one more CI run before landing the patch.