Skip to content

flatten: compile error with copy assignment #3

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

Open
Guekka opened this issue Jan 13, 2022 · 5 comments
Open

flatten: compile error with copy assignment #3

Guekka opened this issue Jan 13, 2022 · 5 comments
Labels
bug Something isn't working

Comments

@Guekka
Copy link
Contributor

Guekka commented Jan 13, 2022

First of all, I know this library is not supposed to be ready for public usage. But I really love the concept, so I had to try it

Of course, I will understand if you do not want to provide support

I am encountering a compile error that I haven't been able to solve. It is easily reproducible. Godbolt link

@Guekka
Copy link
Contributor Author

Guekka commented Jan 14, 2022

Well, I've just discovered lambdas and functions are not copy assignable. Wrapping the function in a shared_ptr inside the map_adapter makes it compile.
Another solution would be to wrap the function in maybe
I can make a PR for one of these solutions, if you are interested

@Guekka
Copy link
Contributor Author

Guekka commented Jan 14, 2022

Actually, the best solution is probably to avoid operator= altogether.

std::optional has emplace. If we add emplace to maybe, we can then use it in place of operator=, and this should fix the issue

@tcbrindle tcbrindle added the bug Something isn't working label Jan 15, 2022
@tcbrindle
Copy link
Owner

No problem filing bug reports! :) Just no promises that I'll be able to fix them...

This is a known (to me) issue that I've run into myself from time to time. If I recall correctly, the reason I used assignment rather than emplace() is that emplace() is non-constexpr in C++17, and cannot be made constexpr because it involves changing the active member of a union (which wasn't allowed until C++20).

At the time I decided that being able to use flatten at compile-time was more important than occasional compile errors when using lambdas, but in hindsight that was probably the wrong trade-off.

@Guekka
Copy link
Contributor Author

Guekka commented Jan 15, 2022

constexpr support is nice too. So it's either constexpr with a heap allocation, or no constexpr and no heap.

I think we should use std::unique_ptr, as a single heap allocation will probably have no significant impact.

If you tell me what you prefer, I can implement it for you

@tcbrindle
Copy link
Owner

I'll need to think about this one -- I don't think allocation should be necessary if we're willing to forego constexpr flatten in C++17 mode, but it would mean adding emplace() to flow::maybe, which it currently doesn't have. There may be other complications as well, I looked it at once before but it's a long time ago and my memory is failing me :).

Leave it with me and I'll see what I can come up with.

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