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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__ranges/range_adaptor.h:48
+_LIBCPP_HIDE_FROM_ABI
+constexpr auto operator|(_View&& __view, _Closure&& __closure)
+noexcept(noexcept(_VSTD::invoke(_VSTD::forward<_Closure>(__closure), _VSTD::forward<_View>(__view))))
----------------
cjdb wrote:
> ldionne wrote:
> > cjdb wrote:
> > > `a | b;` is always a bug for views and is pretty common, not only for folks who are just starting out, but also experienced users too.
> > Isn't this valid?
> > 
> > ```
> > std::vector<int> ints = {...};
> > ints | views::transform([](int i) { std::cout << i; return i + 1; }); // intentionally discard the result
> > ```
> > 
> > Someone using `transform` like that would never pass my code review, but it's valid and I think it's too bold to make that ill-formed.
> Since all views are lazy by definition, that second line is a no-op.
Riiight. Sneaky. :) And is that also true for the (non-standard) pipey things I see in Conor Hoekstra's talks, like `hs::accumulate`? E.g.
```
auto print_and_add = [](int a, int i) { std::cout << a; return a + i; };
ints | hs::accumulate(print_and_add); // intentionally discard the result
```
Of course such things are non-standard and I don't know how they'd be implemented; maybe the answer is just "Such things would be lazy, or else such things wouldn't be implemented in terms of `__range_adaptor_closure`."


================
Comment at: libcxx/include/module.modulemap:673-674
+        private header "__ranges/transform_view.h"
+        export functional.__functional.perfect_forward
+        export functional.__functional.bind_back
+      }
----------------
These exports shouldn't be needed. (If they're needed, something is wrong with these classes. In the case of `__functional_like`, we traced it down to a Clang bug; is that what's wrong here too?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107098