RustCrypto / hybrid-array

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

Add `ArrayOps::to_core_array` #21

Closed thaliaarchi closed 9 months ago

thaliaarchi commented 9 months ago

This adds ArrayOps::to_core_array to parallel ArrayOps::from_core_array and ArrayOps::as_core_array.

I'm using sha1 for working with git internals and wanted to convert GenericArray to [u8; 20]. The latest version of generic-array, v1.0.0 added GenericArray::into_array, which is exactly what I want; however, the current published version of crypto-common uses generic-array v0.14.7. I was going to open an issue there, but found the issues regarding the migration now to hybrid-array and eventually to const generic arrays, so I'm contributing this here instead.

I debated calling it into_core_array to match Rust convention, but followed local convention.

newpavlov commented 9 months ago

Note that Array is defined as struct Array<T, U: ArraySize<T>>(pub U::ArrayType), so you can access the underlying array as arr.0.

thaliaarchi commented 9 months ago

That's true, but that I went down the path of contributing this shows how obvious that was. I guess I usually assume third-party types are opaque, since most don't expose their internal representation. A method like this shows up in autocompletion too.

thaliaarchi commented 9 months ago

Likewise ArrayOps::from_core_array is similarly “useless”, since it just wraps it in the Array newtype.

tarcieri commented 9 months ago

It does indeed seem a bit weird to have from_core_array without a matching to_core_array.

The reason these methods existed was to type erase ArraySize::ArrayType so it doesn't need to be notated everywhere in downstream bounds written by users of the crate, and also because the original implementation strategy had no unsafe code.

However, with an unsafe pointer cast in its place it doesn't need to exist: https://github.com/RustCrypto/hybrid-array/pull/22

tarcieri commented 9 months ago

Also you can use From/Into to convert between the two as well

thaliaarchi commented 9 months ago

If there's going to be from_core_array, there should be to_core_array. Or get rid of them both and use From/Into like your PR. Either's fine. The only downside of Into is that the return type sometimes needs to be annotated.

newpavlov commented 9 months ago

I would prefer to have From/Into impls instead of inherent methods.

tarcieri commented 9 months ago

22 removed from_core_array