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 Slice#unsafe_slice_of, #to_unsafe_bytes #11379

Merged

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Oct 28, 2021

Resolves #11010 , including making #hexdump and #hexstring work for arbitrary Slices.

@oprypin oprypin self-requested a review December 6, 2021 22:35
@oprypin
Copy link
Member

oprypin commented Dec 6, 2021

Hm sorry, of all these things I like only the addition of unsafe_slice_of. It is a frequent operation and is marked as unsafe so that's fine.

  • to_unsafe_bytes is just an alias of unsafe_slice_of(UInt8), not worth the mental overhead.

I would actually put messing with endianness into unsafe territory as well, so my following reasonings are with that in mind:

  • to_bytes returns a system-dependent value without directly acknowledging it in its name. Instead I would suggest an equivalent to it in the docs of to_unsafe_bytes. Which, by the way, you do as Slice.new(to_unsafe.as(UInt8*), bytesize, read_only: true) but couldn't it instead be to_unsafe_bytes.dup?

  • Changing hexstring to become an alias of to_unsafe_bytes.hexstring and returning a system-dependent value is definitely not good. Just suggest the latter piece of code in the docs of hexstring.

@oprypin
Copy link
Member

oprypin commented Dec 6, 2021

And, well, seeing as my latter two items hinge on the existence of to_unsafe_bytes, maybe that one is actually fine.

@HertzDevil HertzDevil changed the title Add Slice#unsafe_slice_of, #to_unsafe_bytes, #to_bytes Add Slice#unsafe_slice_of, #to_unsafe_bytes Dec 7, 2021
@straight-shoota straight-shoota added this to the 1.3.0 milestone Dec 7, 2021
@straight-shoota straight-shoota merged commit b8f0857 into crystal-lang:master Dec 9, 2021
@HertzDevil HertzDevil deleted the feature/slice-to-bytes branch December 10, 2021 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reinterpreting a Slice's elements
3 participants