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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/__ranges_enable_helpers:10
+
+#ifndef _LIBCPP___RANGES_ENABLE_HELPERS
+#define _LIBCPP___RANGES_ENABLE_HELPERS
----------------
I don't think we ever use triple underscore. This makes it hard to read. I think single underscore here is sufficient. 


================
Comment at: libcxx/include/module.modulemap:414
+    header "ranges"
+    export __ranges_enable_helpers
+    export *
----------------
Mordante wrote:
> Quuxplusone wrote:
> > We don't do this line for any other header, e.g. `__functional_base`, so I don't think it should be necessary here.
> I looked at `__string`, but that seems to be the exception. So I'll remove this one.
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?


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


================
Comment at: libcxx/include/ranges:40
+
+namespace ranges
+{
----------------
Nit: I don't think it makes sense to have an empty namespace here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90999