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

readdlm(bytearray) shouldn't modify bytearray #32255

Merged
merged 2 commits into from
Jun 8, 2019

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Jun 6, 2019

Fixes bug reported in #16731 (comment) by @bkamins.

(Would be nice to do this without making a copy of the data, but I don't see any way to do this without adding a new C routine.)

@stevengj stevengj added bugfix This change fixes an existing bug backport 1.0 labels Jun 6, 2019
@bkamins
Copy link
Member

bkamins commented Jun 6, 2019

Actually unsafe_wrap you mentioned in the other thread was considered by @JeffBezanson some time ago if I remember correctly, but I do not recall what was the the reason the idea was dropped (probably safety).

@JeffBezanson
Copy link
Member

We have unsafe_wrap(Vector{UInt8}, ::String), so we should add a method that converts the other way. Maybe we should merge this PR for release branches, and add the new unsafe_wrap method and use it on master?

@JeffBezanson
Copy link
Member

Oh wait, of course that isn't possible in general. And in fact, the current code already makes a copy sometimes, if the Vector isn't backed by a String. We can instead change it to always make 1 copy (instead of 1 or 2) using String(copyto!(Base.StringVector(length(v)), v)).

Co-Authored-By: Jeff Bezanson <jeff.bezanson@gmail.com>
@stevengj
Copy link
Member Author

stevengj commented Jun 8, 2019

Good to merge?

@bkamins
Copy link
Member

bkamins commented Jun 8, 2019

Fine with me.

@StefanKarpinski StefanKarpinski merged commit 7038210 into master Jun 8, 2019
@fredrikekre fredrikekre deleted the readdlm_bytearray branch June 8, 2019 18:17
KristofferC pushed a commit that referenced this pull request Jun 9, 2019
* readdlm(bytearray) shouldn't modify bytearray

* Update stdlib/DelimitedFiles/src/DelimitedFiles.jl

Co-Authored-By: Jeff Bezanson <jeff.bezanson@gmail.com>
(cherry picked from commit 7038210)
@KristofferC KristofferC mentioned this pull request Jun 9, 2019
32 tasks
KristofferC pushed a commit that referenced this pull request Aug 26, 2019
* readdlm(bytearray) shouldn't modify bytearray

* Update stdlib/DelimitedFiles/src/DelimitedFiles.jl

Co-Authored-By: Jeff Bezanson <jeff.bezanson@gmail.com>
(cherry picked from commit 7038210)
@KristofferC KristofferC mentioned this pull request Aug 26, 2019
55 tasks
KristofferC pushed a commit that referenced this pull request Aug 26, 2019
* readdlm(bytearray) shouldn't modify bytearray

* Update stdlib/DelimitedFiles/src/DelimitedFiles.jl

Co-Authored-By: Jeff Bezanson <jeff.bezanson@gmail.com>
(cherry picked from commit 7038210)
KristofferC pushed a commit that referenced this pull request Feb 20, 2020
* readdlm(bytearray) shouldn't modify bytearray

* Update stdlib/DelimitedFiles/src/DelimitedFiles.jl

Co-Authored-By: Jeff Bezanson <jeff.bezanson@gmail.com>
(cherry picked from commit 7038210)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants