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

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

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
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.

@nsajko nsajko added the arrays [a, r, r, a, y, s] label May 6, 2024
vtjnash pushed a commit that referenced this issue Oct 25, 2024
A more targeted fix of #54369 than #54372

Preserves the performance improvements added in #53962 by creating a new
internal `_unsafe_takestring!(v::Memory{UInt8})` function that does what
`String(::Memory{UInt8})` used to do.
@vtjnash vtjnash closed this as completed Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants