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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/module.modulemap:414
+    header "ranges"
+    export __ranges_enable_helpers
+    export *
----------------
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.


================
Comment at: libcxx/include/ranges:15
+
+#include <compare>              // see [compare.syn]
+#include <initializer_list>     // see [initializer.list.syn]
----------------
cjdb wrote:
> Mordante wrote:
> > miscco wrote:
> > > I am slightly surprised that concepts is not required
> > Agreed. For this patch I don't need concepts, but `std::ranges::begin()` will require them.
> It's transitively included by `<iterator>`.
Agreed, but this mandated by the standard. Since the overhead is minimal I prefer to follow the standard. If in a future revision `<initializer_list>` no longer requires `<compare>` we don't silently break our implementation.


================
Comment at: libcxx/test/std/ranges/range.range/enable_borrowed_range.compile.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
cjdb wrote:
> Please change to `enable_borrowed_range.pass.cpp` (we've agreed in other patches to move away from `.compile`). Same with other files (this is just the first I noticed).
In the past had requests to rename test to use `.compile`. I asked for a policy on its usage on Discord, depending on the answer there I'll keep this name or rename the file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90999