[libcxx-commits] [PATCH] D107098: [libc++] Implement the underlying mechanism for range adaptors

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 2 inline comments as done.
ldionne added inline comments.


================
Comment at: libcxx/include/__ranges/range_adaptor.h:60-61
+        requires same_as<_Tp, remove_cvref_t<_Closure>> &&
+                 constructible_from<decay_t<_Closure>, _Closure> &&
+                 constructible_from<decay_t<_OtherClosure>, _OtherClosure>
+    [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
----------------
cjdb wrote:
> ```
> constructible_from<decay_t<_Closure>, _Closure> &&
> constructible_from<decay_t<_OtherClosure>, _OtherClosure>
> ```
> should be rewritten as
> ```
> move_constructible<decay_t<_Closure>> && move_constructible<decay_t<_OtherClosure>>
> ```
> but I don't think that's what we want anyway.
Per the live call we just did, I think the current code is correct (we agreed).


================
Comment at: libcxx/include/__ranges/transform_view.h:414
+    noexcept(noexcept(transform_view(_VSTD::forward<_Range>(__range), _VSTD::forward<_Fn>(__f))))
+    -> decltype(      transform_view(_VSTD::forward<_Range>(__range), _VSTD::forward<_Fn>(__f)))
+    { return          transform_view(_VSTD::forward<_Range>(__range), _VSTD::forward<_Fn>(__f)); }
----------------
cjdb wrote:
> ldionne wrote:
> > cjdb wrote:
> > > Same dealeo as above: this should be a requirement, not expression-SFINAE. Gonna leave it at three comments, I trust you to find all the rest :-)
> > For this one, http://eel.is/c++draft/range.adaptors#range.transform.overview-2 says it should be expression equivalent to `transform_view(range, f)`. But you're correct in general for the other ones and I changed a bunch.
> I have a bad feeling about the diagnostics this is gonna produce. Hopefully I'm wrong.
Yeah, writing expression-equivalent stuff leads to not-super-great error messages, but I still think it's the correct way to do it in this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107098