Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 #54372base: master
Are you sure you want to change the base?
Add takestring! and make
String(::Memory)
not truncate #54372Changes from 33 commits
bd1677e
71a030f
d247d92
699916e
1ab1785
50d62c8
9e14b92
55f8d23
46d0357
2f36912
d7174bf
94485e6
a28c45d
ba78f83
ac5753b
4f838cd
1ef6b59
f3046c0
1047577
513038e
ec6e128
1d67150
1a498f4
7c7ce9c
0b9aff5
8fc3cfd
9948ec5
176506c
1689258
998c9f6
c9bfe09
67b4d9a
d7ff276
462fcae
35fe11c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 thetake!
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 totrue
, and thus the returned value is an alias of the memory in the buffer. However, this is only safe is one can guarantee the returnedVector
is never mutated. We can guarantee that when returningString
, 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.
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.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 fallback isn't needed.
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.
Why not?
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 removed it and the tests passed locally. There are also no functions that call
unsafe_takestring!
on aGenericIOBuffer
.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.
A problem could arise if you construct a
GenericIOBuffer
with a backing buffer that is notVector{UInt8}
. I'm not sure if this is exercised by the tests.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.
unsafe_takestring!
is an internal function, so it shouldn't be an issue.