RustCrypto / hybrid-array

Hybrid typenum/const generic arrays
Apache License 2.0
9 stars 8 forks source link

bring back `zip` methods #60

Closed baloo closed 8 months ago

baloo commented 8 months ago

See example usage: https://github.com/RustCrypto/AEADs/blob/d1a22983d2af3c6a1ac3644850351f0ab0a77e51/eax/src/online.rs#L352

baloo commented 8 months ago

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 commented 8 months ago

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.

tarcieri commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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.

tarcieri commented 8 months ago

Where is it actually being used?

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

baloo commented 8 months ago

generic-array: https://github.com/RustCrypto/AEADs/blob/d1a22983d2af3c6a1ac3644850351f0ab0a77e51/eax/src/online.rs#L352

hybrid-array (earlier implementation): https://github.com/RustCrypto/AEADs/blob/00dbd8a8d15895a6df8759b2fbf7025b2c57eb52/eax/src/online.rs#L352

tarcieri commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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()

tarcieri commented 8 months ago

FWIW bringing back map still seems good to me

baloo commented 8 months ago

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

tarcieri commented 8 months ago

I don't have a branch

baloo commented 8 months ago

https://github.com/RustCrypto/hybrid-array/tree/array-map ??