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

String(v::Memory{UInt8}) doesn't make a copy #54369

Open
nhz2 opened this issue May 5, 2024 · 1 comment · May be fixed by #54372
Open

String(v::Memory{UInt8}) doesn't make a copy #54369

nhz2 opened this issue May 5, 2024 · 1 comment · May be fixed by #54372
Labels
domain:arrays [a, r, r, a, y, s]

Comments

@nhz2
Copy link
Contributor

nhz2 commented May 5, 2024

The docstring of String says:

To avoid truncation
of Vector{UInt8} data, use String(copy(v)); for other AbstractVector types,
String(v) already makes a copy.

Since Memory is an AbstractVector, String should make a copy. However, it doesn't look like this is happening in:

function String(v::Memory{UInt8})
len = length(v)
len == 0 && return ""
return ccall(:jl_genericmemory_to_string, Ref{String}, (Any, Int), v, len)
end

julia> v = Memory{UInt8}(undef, 10);

julia> fill!(v, 0x01);

julia> String(v)
"\x01\x01\x01\x01\x01\x01\x01\x01\x01\x01"

julia> v
0-element Memory{UInt8}

Maybe the current String(v::Memory{UInt8}) method could be renamed to something like take_string! to make it clear that it can mutate v.

@jakobnissen
Copy link
Contributor

I think it's pretty much agreed that the truncating behaviour of String(::Vector{UInt8}) was a design mistake: #32528. It'd be unfortunate to repeat this with Memory, given that there is still time before 1.11 is released.

@jakobnissen jakobnissen linked a pull request May 6, 2024 that will close this issue
2 tasks
@nsajko nsajko added the domain:arrays [a, r, r, a, y, s] label May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants