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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


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


================
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> >);
+
----------------
Same thing tested twice in a row. What was meant here? Ditto twice in the lines below.


================
Comment at: libcxx/test/std/containers/views/enable_borrowed_range.compile.pass.cpp:35
+  static_assert(enable_borrowed_range<span<const volatile T, 1> >);
+  static_assert(enable_borrowed_range<span<const volatile T, 1> >);
+
----------------
Again, I recommend using something other than `1` as your "regular ordinary span" type. I suggest `10`.


================
Comment at: libcxx/test/std/language.support/support.limits/support.limits.general/ranges.version.pass.cpp:10
+// WARNING: This test was generated by generate_feature_test_macro_components.py
+// and should not be edited manually.
+//
----------------
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.


================
Comment at: libcxx/test/std/ranges/range.range/enable_borrowed_range.compile.pass.cpp:42
+  static_assert(!enable_borrowed_range<std::array<int, 0> >);
+  static_assert(!enable_borrowed_range<std::array<int, 1> >);
+  static_assert(!enable_borrowed_range<std::deque<int> >);
----------------
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)".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90999