-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 takestring! and make String(::Memory)
not truncate
#54372
base: master
Are you sure you want to change the base?
Conversation
Also, check for uses of julia/stdlib/FileWatching/src/pidfile.jl Lines 283 to 288 in f3561ae
#53962 |
The name makes me think that |
I would be in favor of that. Every time I have used an IOBuffer I have had to go to the docs to figure out how to get the data out of it correctly. |
So what's the specification of |
It does not truncate - except when the argument is a |
Do we need an underscore here? |
Triage had a long talk about this, #54369, #54156 and #54273. The conclusions we came to are
|
The style guide says not to use underscores "when readable" without, which I think applies here. |
String(::Memory)
not truncateString(::Memory)
not truncate
Can this be refined to "mutating an Array after its alias has been turned into a String is UB"? edit: For the record , I don't think it's right for us to put this contract on an API that's not marked |
Yes. observing it is not great but not UB. the only UB is if you mutate it. |
|
After triage, this PR now contains the following changes:
|
This commit builds upon JuliaLang#54438, re-using the old String(::Vector{UInt8}) impl, which now no longer truncates. Also remove takestring!(::Memory) - that function probably should not be defined
Co-authored-by: Steven G. Johnson <stevenj@mit.edu>
And use it unstead of takestring!(::Memory)
* Do not assume io.seekable in unsafe_takestring!(::IOBuffer) * Add fallback definitions unsafe_takestring!(::GenericIOBuffer) and takestring!(::GenericIOBuffer) * Test takestring! with a nonzero offset in buffer
Specifically, do not use it when a value of an unknown type prints to the buffer that is being mutated, since any user may define printing of these values to store references to the buffer somewhere else.
8a7667f
to
67b4d9a
Compare
# we can return an empty string without interacting with the buffer at all. | ||
io.reinit && return "" | ||
|
||
s = unsafe_takestring!(io) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the buffer is not writable, this must copy the data into a StringMemory
, see line 471 in the take!
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, but perhaps I'm wrong. The reason it copies in line 471 is because, if the buffer is not writable, io.reinit
is not set to true
, and thus the returned value is an alias of the memory in the buffer. However, this is only safe is one can guarantee the returned Vector
is never mutated. We can guarantee that when returning String
, so it's fine that the returned string shares memory with the unwriteable buffer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will normally be fine, but the following example seg faults with this PR.
iow = IOBuffer()
write(iow, zeros(UInt8, 10_000_000))
v1 = take!(iow)
ior = IOBuffer(v1)
s1 = takestring!(ior)
s1 = nothing
GC.gc()
GC.gc()
GC.gc()
v1[1]
This is because after a call to unsafe_takestring(m::Memory{UInt8})
m
becomes unsafe to read because its memory may get freed by Julia if the string isn't needed.
@@ -54,6 +54,26 @@ bufcontents(io::Base.GenericIOBuffer) = unsafe_string(pointer(io.data), io.size) | |||
@test_throws ArgumentError seek(io, 0) | |||
end | |||
|
|||
@testset "takestring!" begin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be tested for all the different kinds of IOBuffer
This function creates a string from a byte vector, truncating the vector and reusing its memory where possible.
This also removes the truncating behaviour of
String(::Memory)
- see #54369Still needs to be done
take_string!(::Memory)
should be used instead ofString(::Memory)
for efficiency [edit: couldn't find any]Closes #54369