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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 2 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/__ranges_enable_helpers:10
+
+#ifndef _LIBCPP___RANGES_ENABLE_HELPERS
+#define _LIBCPP___RANGES_ENABLE_HELPERS
----------------
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.


================
Comment at: libcxx/include/__ranges_enable_helpers:38
+
+#if _LIBCPP_STD_VER > 17
+
----------------
cjdb wrote:
> We've got a `_LIBCPP_HAS_NO_RANGES` macro now that guards on C++20 now.
Thanks for the information, will adjust this code.


================
Comment at: libcxx/include/ranges:38
+
+#if _LIBCPP_STD_VER > 17
+
----------------
zoecarver wrote:
> Same as the other comment, you can use `_LIBCPP_HAS_NO_RANGES` (in fact, let's guard the whole header on that). 
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`.


================
Comment at: libcxx/test/std/containers/views/enable_borrowed_range.compile.pass.cpp:29-30
+  static_assert(enable_borrowed_range<span<const T, 0> >);
+  static_assert(enable_borrowed_range<span<const volatile T, 0> >);
+  static_assert(enable_borrowed_range<span<const volatile T, 0> >);
+
----------------
Quuxplusone wrote:
> Same thing tested twice in a row. What was meant here? Ditto twice in the lines below.
It seems I wanted to test `volatile` but made a typical classical copy-paste error.


================
Comment at: libcxx/test/std/containers/views/enable_borrowed_range.compile.pass.cpp:40
+  static_assert(enable_borrowed_range<span<const volatile T, dynamic_extent> >);
+  static_assert(enable_borrowed_range<span<const volatile T, dynamic_extent> >);
+}
----------------
Dito.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90999