[libcxx-commits] [PATCH] D102020: [libcxx][ranges] Add class ref_view.

Wed Jun 9 11:59:30 PDT 2021

ldionne accepted this revision.
ldionne added a comment.

LGTM. Please make `__binds_to_lvalue_ref` private, and we might want to rename it to something like `__avoid_ICS_from_rvalue` or something like that.

================
Comment at: libcxx/include/__ranges/ref_view.h:40-41
+
+    static void __test(_Range&);
+    static void __test(_Range&&) = delete;
+
----------------
tcanens wrote:
> ldionne wrote:
> > Those can be made private, and I would perhaps rename it to something like `__binds_to_lvalue_ref` or something like that.
> > 
> > Do you have a better understanding of what this function is used for than I do? IIUC, it's only used to check that `__tp` in the constructor below would bind to a lvalue, but not to a rvalue. I'm kind of wondering why the Standard is using this obtuse wording -- there must be a good reason?
> This is the [LWG2993](https://cplusplus.github.io/LWG/issue2993) dance to avoid having an ICS from rvalues.
Thanks for the context. I must say though, I was unable to construct a case where dropping `requires { __binds_to_lvalue_ref(declval<_Tp>()); }` would make a difference since we're already checking for `convertible_to<_Tp, _Range&>`.

In LWG2993, I understand why `reference_wrapper` used to be broken with the `reference_wrapper(T&&) = delete` overload, but I fail to see how this applies here. I guess it's not relevant to this review, I'm just curious in case you have time to elaborate.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.ref.view.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
I just want to say: normally, we'd split this into several files, one per tested thing. But `ref_view` is almost only its synopsis, so I think this is fine.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102020