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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver accepted this revision as: zoecarver.
zoecarver added a comment.

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



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90999