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

correct String(array) docs #25846

Merged
merged 12 commits into from
Nov 2, 2020
Merged

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Feb 1, 2018

I noticed that the String(::Vector{UInt8}) docs stated that String(array) always takes "ownership" of the data. This is no longer true in 0.7 except for arrays allocated with StringVector.

This PR corrects the String(array) docs to accurately describe the circumstances in which String takes ownership of the array (currently take! and read are the only documented ways as far as I can tell), and the fact that the length of the array is set to 0 in such cases.

Also, since String generally makes a copy for an arbitrary Vector{UInt8}, I saw no reason not to expand the constructor to accept AbstractVector{UInt8} (previously a MethodError). (Whoops, this method was already added.)

See also #24388 (comment)

@stevengj stevengj added the domain:strings "Strings!" label Feb 1, 2018
@stevengj
Copy link
Member Author

stevengj commented Feb 1, 2018

(I tend to think we should document StringVector too.)

@stevengj stevengj added the domain:docs This change adds or pertains to documentation label Feb 1, 2018
@JeffBezanson
Copy link
Sponsor Member

This method already exists a few lines below.

@stevengj
Copy link
Member Author

stevengj commented Feb 2, 2018

Whoops, I didn't notice that you already added that method; I removed the redundancy.

@stevengj stevengj changed the title correct String(array) docs and generalize to String(::AbstractVector) correct String(array) docs Feb 2, 2018
@nalimilan
Copy link
Member

Thanks for documenting this. Though I must say that reading the description of the behavior, it doesn't feel satisfying. Apart from being inconsistent, it's dangerous because you can't know whether a vector passed by a caller has been allocated by e.g. read or not. So if you want to be safe, you need to make a copy, which most of the time will result in making two copies... That behavior doesn't seem to correspond to what was discussed at #24388, does it?

That said, better merge the PR since it describes what currently happens.

@StefanKarpinski
Copy link
Sponsor Member

cc @JeffBezanson

@JeffBezanson
Copy link
Sponsor Member

That behavior doesn't seem to correspond to what was discussed at #24388, does it?

That issue was about preventing mutation of strings via vectors, which the current behavior accomplishes.

@stevengj
Copy link
Member Author

Resolved a merge conflict. Okay to merge?

@stevengj
Copy link
Member Author

stevengj commented Nov 2, 2020

This PR is mostly obsolete, and now boils down to a 1-line clarification to the docs. Should be fine to merge.

@stevengj stevengj added this to the 1.6 features milestone Nov 2, 2020
@stevengj stevengj merged commit 7e5699d into JuliaLang:master Nov 2, 2020
@stevengj stevengj deleted the stringconstructor branch November 2, 2020 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:docs This change adds or pertains to documentation domain:strings "Strings!"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants