[libcxx-commits] [PATCH] D90999: [libc++] Implements ranges::enable_borrowed_range

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

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.



================
Comment at: libcxx/include/__ranges_enable_helpers:10
+
+#ifndef _LIBCPP___RANGES_ENABLE_HELPERS
+#define _LIBCPP___RANGES_ENABLE_HELPERS
----------------
zoecarver wrote:
> Mordante wrote:
> > Quuxplusone wrote:
> > > zoecarver wrote:
> > > > I don't think we ever use triple underscore. This makes it hard to read. I think single underscore here is sufficient. 
> > > 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.
> > 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.
> Good to know. In that case I'm fine with triple underscore, or whatever's appropriate for the header name we land on. 
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.


================
Comment at: libcxx/include/string_view:658
+template <class _CharT, class _Traits>
+inline _LIBCPP_INLINE_VISIBILITY constexpr
+bool _VSTD::ranges::enable_borrowed_range<
----------------
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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90999/new/

https://reviews.llvm.org/D90999