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

remove the use of String's len field (introduced in #22133) #22756

Merged
merged 1 commit into from
Jul 11, 2017

Conversation

rfourquet
Copy link
Member

The len field has been removed in 2b47865.
I assume here that jl_alloc_string (called by _string_n) reserves one char more for \0 that some library will insert, as far as I can understand its code (it seems len+1 bytes are allocated, where len is the requested size, the last byte being zeroed).

@rfourquet rfourquet added the bugfix This change fixes an existing bug label Jul 11, 2017
Copy link
Contributor

@yuyichao yuyichao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strings shouldn't be mutated like this. Use StringVector instead.

@timholy
Copy link
Member

timholy commented Jul 11, 2017

I went ahead and canceled a bunch of CI runs on AppVeyor and Travis, since nothing will pass until this gets through.

@JeffBezanson
Copy link
Member

This must mean there are no tests for this function? Could you add some?

@JeffBezanson
Copy link
Member

I see, I guess there are tests. I didn't realize this code changed so recently.

@rfourquet
Copy link
Member Author

Strings shouldn't be mutated like this.

I thougth this was the purpose of _string_n (it's used like this in "serialize.jl" and "strings/string.jl", as far as I can tell...)

@rfourquet
Copy link
Member Author

replaced _string_n by StringVector

@JeffBezanson JeffBezanson dismissed yuyichao’s stale review July 11, 2017 19:24

Review comment was addressed.

@JeffBezanson JeffBezanson merged commit 37ca8c7 into master Jul 11, 2017
@timholy
Copy link
Member

timholy commented Jul 11, 2017

@rfourquet, thanks for taking the lead on this. These kinds of "distributed errors" aren't anyone's fault and are unavoidable when changes are happening at a rate that exceeds individual-human bandwidth. Having someone who takes ownership and deals with it so quickly is simply priceless.

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.

4 participants