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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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))))
----------------
Quuxplusone wrote:
> 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`."
> Since all views are lazy by definition, that second line is a no-op.

You're right, and the fact that I had that brainfart justifies adding `[[nodiscard]]` IMO.


================
Comment at: libcxx/include/module.modulemap:673-674
+        private header "__ranges/transform_view.h"
+        export functional.__functional.perfect_forward
+        export functional.__functional.bind_back
+      }
----------------
Quuxplusone wrote:
> 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?)
Honestly, I'm not too sure because the errors are way too cryptic. However, adding them fixes issues we have in the modular build, so I think it's OK to do it, as we do elsewhere. The current state of modules makes it so that I think it's reasonable to make a few blind fixes if it resolves issues and allows us to ship patches. Once the QOI has increased, we can revisit the whole modulemap and figure out what actually needs to be there and make those changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107098