tcbrindle / flux

A C++20 library for sequence-orientated programming
https://tristanbrindle.com/flux/
Boost Software License 1.0
479 stars 29 forks source link

Zip ints with filtered vector not working #47

Closed jnytra closed 1 year ago

jnytra commented 1 year ago

I'm trying zip ints() sequence with the filtered vector, but I am getting compilation error.

std::vector v = {1, 2, 3, 4, 5};
auto res = flux::zip(flux::ints(), flux::from(v).filter(flux::pred::gt(3)));

https://godbolt.org/z/oaEn1vfr7

tcbrindle commented 1 year ago

Thanks for submitting Flux's first ever bug report! 🥇

This example should definitely work -- it turns out that the ints() sequence isn't the problem, but it's actually the filter adaptor tripping up a concept definition.

I'll get it fixed, but in the mean time a workaround is to save the filtered sequence to a temporary variable, and pass a reference to that variable into zip:

https://godbolt.org/z/fn7Pzzdcv

tcbrindle commented 1 year ago

There are a couple of things going on here.

The first problem is that the rvalue_sequence<S> concept is checking whether S is std::movable -- this fails because the pred::lt object is not move-assignable (as it's effectively a capturing lambda). But we don't really need to check for std::movable, what we're actually interested in is whether the sequence std::move_constructible, so I'll need to change this concept definition. (As a workaround for this, you can replace the pred::lt with a non-capturing lambda like auto gt3 = [](int i) { return i > 3; };.)

Unfortunately, using this workaround reveals another problem, in the range-for loop. Specifically calling res.begin() tries to instantiate both begin() and begin() const on the zip_adaptor object, and the const version will hard error because the filter_adaptor it contains is not const-iterable. The solution for this is for zip_adaptor to properly constrain (or SFINAE) its first/is_last/etc implementations, so that they disappear from overload consideration when the passed-in zip adaptor is const but it contains non-const-iterable things (at present they're unconstrained and just return auto).

C++ is hard! But this is why it's good to have users :)

tcbrindle commented 1 year ago

This should now be fixed. Thanks again for the bug report!