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

bring back zip methods #60

Closed
wants to merge 2 commits into from
Closed

Conversation

baloo
Copy link
Member

@baloo baloo commented Mar 4, 2024

@baloo
Copy link
Member Author

baloo commented Mar 4, 2024

The only purpose of using a trait here is to limit API breakage with generic-array but it's otherwise not required.

Now I comment on it, I kind of feel I should have gone with a straight impl Array block. Opinion?

@tarcieri
Copy link
Member

tarcieri commented Mar 4, 2024

Yes, these should just be inherent methods. GenericArray was weird for defining traits for everything, being the only implementor, and then writing blanket impls, which only makes it annoying to pull those traits into scope.

No unsafe code should be required for any of these. You should be able to leverage the FromIterator/IntoIterator impls, either on core arrays or on Array, e.g. Array::map can be written Array(self.0.map(f)).

It'd also be good to update the migration guide.

src/functional.rs Outdated Show resolved Hide resolved
src/functional.rs Outdated Show resolved Hide resolved
@baloo baloo changed the title bring back FunctionalSequence bring back map/zip methods Mar 4, 2024
@tarcieri
Copy link
Member

tarcieri commented Mar 4, 2024

Perhaps we can just add Array::map and for zip and join suggest using IntoIterator instead in the migration guide: https://github.com/RustCrypto/hybrid-array/blob/b362ec7/src/lib.rs#L66

@tarcieri
Copy link
Member

tarcieri commented Mar 4, 2024

map looks good, but zip seems weird to me, as in there's [T; N]::map defined in core, but no corresponding zip method in core.

I'm not sure this really deserves its own module either, especially if it's just map.

@baloo
Copy link
Member Author

baloo commented Mar 4, 2024

well, we could do a into_iter().zip(other_array) but this doesn't strictly enforces the size which would be missing.
https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.zip.

I don't know how to write a migration guide without just copy pasting this implementation every time.

@baloo baloo changed the title bring back map/zip methods bring back zip methods Mar 4, 2024
@tarcieri
Copy link
Member

tarcieri commented Mar 4, 2024

Where is it actually being used?

If we do add it, I think it should be deprecated.

@baloo
Copy link
Member Author

baloo commented Mar 4, 2024

@tarcieri
Copy link
Member

tarcieri commented Mar 4, 2024

Yikes, that's gross!

let full_tag = self.nonce.zip(h, |a, b| a ^ b).zip(c, |a, b| a ^ b);
Tag::<M>::clone_from_slice(&full_tag[..M::to_usize()])

It looks like it could be replaced by something like:

self.nonce.into_iter().zip(h, |a, b| a ^ b).take(M::to_usize()).collect()

@tarcieri
Copy link
Member

tarcieri commented Mar 4, 2024

I guess I should also mention: one of the goals of hybrid-array is to be shaped a lot closer to core arrays, to aid an eventual transition to core arrays.

I would prefer not to continue to provide generic-array esoterisms, and if we do they should only be to aid a transition, and deprecated.

@baloo
Copy link
Member Author

baloo commented Mar 4, 2024

the only benefit I've seen from having a zip implementation was the enforcement of the sizes.

But in the case of eax, this is enforced externally (message and data being the same size / type).
Yeah. I guess we can go with the into_iter()

@baloo baloo closed this Mar 4, 2024
@baloo baloo deleted the baloo/functional branch March 4, 2024 19:16
@tarcieri
Copy link
Member

tarcieri commented Mar 4, 2024

FWIW bringing back map still seems good to me

@baloo
Copy link
Member Author

baloo commented Mar 4, 2024

well, it was in your branch?
I didn't a use for it quite yet.

@tarcieri
Copy link
Member

tarcieri commented Mar 4, 2024

I don't have a branch

@baloo
Copy link
Member Author

baloo commented Mar 4, 2024

@baloo baloo mentioned this pull request Mar 4, 2024
tarcieri added a commit that referenced this pull request Mar 5, 2024
Alternative to #60

Co-authored-by: Tony Arcieri <bascule@gmail.com>
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.

None yet

2 participants