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

Add macro iunpack #841

Closed
wants to merge 3 commits into from
Closed

Add macro iunpack #841

wants to merge 3 commits into from

Conversation

A4-Tacks
Copy link

Add it, simply and quickly unpack iterators to patterns.

@phimuemue
Copy link
Member

Hi there, this is some impressive macro-programming.

But from a user's perspective: Is this really better than a manual collect_tuple (and its cousins), or - in the special cases - manually collecting the iterator into separate variables? iunpack requires the user to learn yet another mini-language (see the *, **, *c=, etc). We could justify this if the macro covered all use cases - but e.g. can it e.g. unpack into a HashSet, but only values that match the pattern 1 | 2? And if so, how easy would it be to specify this?

I think itertools should provide (relatively) fool-proof and intuitive helpers, and I don't think iunpack fits this category. So my gut feeling would be to decline this PR.

@A4-Tacks
Copy link
Author

Hi there, this is some impressive macro-programming.

But from a user's perspective: Is this really better than a manual collect_tuple (and its cousins), or - in the special cases - manually collecting the iterator into separate variables? iunpack requires the user to learn yet another mini-language (see the *, **, *c=, etc). We could justify this if the macro covered all use cases - but e.g. can it e.g. unpack into a HashSet, but only values that match the pattern 1 | 2? And if so, how easy would it be to specify this?

I think itertools should provide (relatively) fool-proof and intuitive helpers, and I don't think iunpack fits this category. So my gut feeling would be to decline this PR.

I think it's easy to understand, and we have macro expand, right?
Its operation is like extending the slice pattern to iterators

@A4-Tacks
Copy link
Author

For example, it can unpack structures or verify the correctness of enumerations when unpacking iterators, which will be extremely convenient and can simplify many codes

@A4-Tacks
Copy link
Author

And this macro implementation can effectively handle iterators with variable numbers of elements for quick single branch matching at the beginning and end, without the need for lengthy code.
One of the powerful features of this macro is the use of buffers to rely solely on one-way iterators to match tails

@Philippe-Cholet
Copy link
Member

It seems powerful but quite unusual at first sight, and all those loop-break around the macro (but I guess it could be presented differently). I get that you understand everything here as you made it but it's not that easy. I would agree with phimuemue.

After looking at the internals, I need to say that we currently use stable Rust 1.43.1 and "let-else" is not available for us yet (and 1.65 is not for tomorrow).

Honestly after all the debate of iproduct!(something; n) (#264) that should be simple enough (which I intend to solve in a near future), this definitely seems on another level.

@A4-Tacks
Copy link
Author

A4-Tacks commented Jan 10, 2024

It seems powerful but quite unusual at first sight, and all those loop-break around the macro (but I guess it could be presented differently). I get that you understand everything here as you made it but it's not that easy. I would agree with phimuemue.

After looking at the internals, I need to say that we currently use stable Rust 1.43.1 and "let-else" is not available for us yet (and 1.65 is not for tomorrow).

Honestly after all the debate of iproduct!(something; n) (#264) that should be simple enough (which I intend to solve in a near future), this definitely seems on another level.

Regarding the issue of let-else, may it be possible to modify it to an internal code block?
I still hope it will be merged.

@A4-Tacks
Copy link
Author

It seems powerful but quite unusual at first sight, and all those loop-break around the macro (but I guess it could be presented differently). I get that you understand everything here as you made it but it's not that easy. I would agree with phimuemue.

After looking at the internals, I need to say that we currently use stable Rust 1.43.1 and "let-else" is not available for us yet (and 1.65 is not for tomorrow).

Honestly after all the debate of iproduct!(something; n) (#264) that should be simple enough (which I intend to solve in a near future), this definitely seems on another level.

Having changed it to use if-let, is it available now?

@A4-Tacks
Copy link
Author

Can it run now? Can it be merged?

@Philippe-Cholet
Copy link
Member

I just let our CI run, it passed (thanks to the removed "let-else") which does not mean it'll be merged.

The "merge process" might be considered slow for new additions and especially for something that is not easy like this. But if we add something, we commit ourselves to maintain it.
I'd first like to know if the changes change @phimuemue's opinion on this as it seems a bit more friendly than initially.

@phimuemue
Copy link
Member

I just let our CI run, it passed (thanks to the removed "let-else") which does not mean it'll be merged.

The "merge process" might be considered slow for new additions and especially for something that is not easy like this. But if we add something, we commit ourselves to maintain it. I'd first like to know if the changes change @phimuemue's opinion on this as it seems a bit more friendly than initially.

I hope I do not come across as rude, but this did not change my opinion. Imho this macro - while impressive - has downsides:

  • It wraps trivial behavior (albeit in a non-trivial way). Sure, nice for code golf, but I think it not desperately needed in general.
  • Too specific: Could I collect the first three elements into a HashMap if they match (1|2), and collect the remaining ones into b, c, d if they are odd numbers, collect the rest into a Vec, and have a dedicated error depending on whether it fails to collect the HashMap, b, c, d or the Vec. No problem with manual collecting, but impossible/not straightforward with this macro.
    A slight change in requirements may thus require users to rewrite from iunpack to manual collect.
  • It promotes an own mini-language that is not capable of expressing sufficiently many use cases, and I think collecting manually is clearer.
  • It is imho not straightforward what this macro does. (Just from a skim, I see it introduces a loop, or it populates an array and uses rotate_left afterwards, while simply skipping the elements would be more performant. All of which is unexpected in my book.)

I think this has its place in a code-golf/AoC-like crate, but not in itertools. But I won't veto if @Philippe-Cholet or @jswrenn wants to merge.

@scottmcm
Copy link
Contributor

iunpack requires the user to learn yet another mini-language

I think this is the biggest things that, personally, pushes me away from saying this is a good fit for itertools.

The other macros that exist are about handling a variable number of heterogeneous parameters, where macros are the only reasonable option today, but which act far more like functions. (chain!(a, b, c) could be made to work as multichain((a, b, c)) with some traits implemented on tuples, for example, instead.)

But iunpack is a DSL, which feels different enough from the other things in here that I think it'd be better in a different crate.

@Philippe-Cholet
Copy link
Member

I think we three agree that it is a nice and powerful macro that does not really fit our crate.
It should belong somewhere, maybe its own crate, as I do not know which crate it could be a good fit.

@phimuemue
Copy link
Member

phimuemue commented Jan 12, 2024

Closing, outside the scope of itertools.

@phimuemue phimuemue closed this Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants