Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Zip ints with filtered vector not working #47

Closed
jnytra opened this issue Apr 17, 2023 · 3 comments
Closed

Zip ints with filtered vector not working #47

jnytra opened this issue Apr 17, 2023 · 3 comments
Labels
bug Something isn't working

Comments

@jnytra
Copy link
Contributor

jnytra commented Apr 17, 2023

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
Copy link
Owner

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 tcbrindle added the bug Something isn't working label Apr 18, 2023
@tcbrindle
Copy link
Owner

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 added a commit that referenced this issue Apr 22, 2023
@tcbrindle
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants