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

bytes2hex(::Union{NTuple{Any, UInt8}, AbstractArray{UInt8}}) is needlessly restrictive #39284

Closed
Seelengrab opened this issue Jan 16, 2021 · 2 comments · Fixed by #39710
Closed

Comments

@Seelengrab
Copy link
Contributor

I've recently had the problem of using bytes2hex on the reverse of a large vector that mustn't be modified (and copying it would be unnecessary, as bytes2hex already allocates). The arguments for bytes2hex are restricted by Union{NTuple{Any, UInt8}, AbstractArray{UInt8}} though, so using Iterators.reverse to save the copying wasn't an option. bytes2hex doesn't require its argument to be an array though - being iterable, having a length and having an eltype of UInt8 is enough.

For my specific problem, adding the following method was enough to fix it:

function bytes2hex(a::Base.Iterators.Reverse{<:AbstractVector{UInt8}})
    b = Base.StringVector(2*length(a))
    @inbounds for (i, x) in enumerate(a)
        b[2i - 1] = hex_chars[1 + x >> 4]
        b[2i    ] = hex_chars[1 + x & 0xf]
    end
    return String(b)
end

This won't work for general iterables of course, which is why I opened this issue instead of a PR widening the type signature. I'm mostly looking for ideas on how to best widen the signature and would submit a PR implementing it once we've decided on a design.

@stevengj
Copy link
Member

We could just define it as bytes2hex(itr) and then throw an error if eltype(itr) != UInt8, I guess?

@Seelengrab
Copy link
Contributor Author

Yes, that's one part - I guess ArgumentError("eltype of iterable not UInt8") would be best?

Hm, general iterables with undefined length would have to return an iterator as well, but those with HasLength could use it to keep the current interface non-breaking 🤔 And the lazy variant could live in Iterators, though I'm not sure we should add that version if the n2x interface is planned to change #14418

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 a pull request may close this issue.

2 participants