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

[Tim Song via Phabricator via libcxx-commits]

tcanens added inline comments.


================
Comment at: libcxx/include/__ranges/ref_view.h:40-41
+
+    static void __test(_Range&);
+    static void __test(_Range&&) = delete;
+
----------------
ldionne wrote:
> Quuxplusone wrote:
> > ldionne wrote:
> > > 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.
> > I believe the only case this catches is when `T` is ambiguously convertible to both `R&` and `R&&`:
> > https://godbolt.org/z/hT4jraEo7
> > 
> > ```
> > struct Evil {
> >     operator std::vector<int>& () const;
> >     operator std::vector<int>&& () const;
> > };
> > static_assert(!std::is_convertible_v<Evil, std::reference_wrapper<std::vector<int>>>);
> > static_assert(!std::is_convertible_v<Evil, std::ranges::ref_view<std::vector<int>>>);
> > ```
> > 
> > I don't think there was ever a good reason for `reference_wrapper` to not-bind-to-rvalues, but it's true that `reference_wrapper` and `ref_view` are doing the exact same dance here and for exactly isomorphic reasons. (And that's a good reason to keep the naming convention the same, btw. `reference_wrapper` calls the helper function `__fun`, and I think we should do exactly the same here.)
> > calls the helper function __fun, and I think we should do exactly the same here
> 
> Yeah, I considering that when I wrote my comment and I think the fact that you're suggesting it is enough to sway me. @zoecarver , if you agree, I would go back to the not-very-descriptive-but-consistent-and-close-to-the-standard `__fun` naming you had initially.
> 
> Perhaps you could also add a test like what Arthur suggested?
`_Range` can be const-qualified, and then `_Range&` can bind to rvalues, which is what this is trying to disallow.

I'm not sure if this actually //needs// the 2993 do-not-even-have-an-ICS treatment though (as opposed to `= delete;`). It's pretty straightforward to come up with toy examples where it makes a difference, but realistic examples are harder to come by.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102020