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

Rename Memory wrapper from view to unsafe_vector #54273

Closed
wants to merge 2 commits into from

Conversation

nhz2
Copy link
Contributor

@nhz2 nhz2 commented Apr 26, 2024

Fixes #54156
Changes part of #53896

The main difference between view and unsafe_vector is that the result of view has a fixed size, and creates a copy when passed to String.

The String(view( pattern is fairly common: https://juliahub.com/ui/Search?q=String(view&type=code and as more packages start using Memory it may be hard to tell if view is being used safely or not.

I'm not sure about the name unsafe_vector, but the docstring says

It is only safe to
resize v if m is subseqently not used.

So I added an unsafe_ prefix.

Ref #53552 (comment) for a discussion on why view on a Memory should produce an Array.

@oscardssmith oscardssmith added the status:triage This should be discussed on a triage call label Apr 26, 2024
@christiangnrd
Copy link
Contributor

If this gets approved, it might be worth adding a "See also:" reference to unsafe_vector in the view docstring.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 26, 2024

Nit: nothing about this is unsafe, so we could call it wrap as the counter part to unsafe_wrap

@adienes
Copy link
Contributor

adienes commented Apr 26, 2024

wasn't it originally wrap then reverted?

#53896

I might be conflating issues together, didn't follow super closely

@nhz2
Copy link
Contributor Author

nhz2 commented Apr 26, 2024

Isn't Vector allowed to shift the data in its mem when resized? For example, in the future as an optimization, could doing popfirst! then push! cause the data to be shifted over by one in mem?

@longemen3000
Copy link
Contributor

longemen3000 commented Apr 26, 2024

This would surely break CSV.jl, as the removal of wrap did before. I don't have a strong opinion on this, but I'm commenting to know when/if this PR is merged to add the corresponding fix

@jariji
Copy link
Contributor

jariji commented Apr 26, 2024

Triage named it view in #53552.

I don't really understand what the String(_) constructor's contract is supposed to be, especially on the issue of memory ownership and mutation.

@nsajko
Copy link
Contributor

nsajko commented Apr 28, 2024

How about naming it wrap, but not export-ing it?

@jariji
Copy link
Contributor

jariji commented Apr 29, 2024

"Base.wrap" doesn't seem like it means anything in particular, it's too uninformative. At least that was my view and I think triage agreed.

@oscardssmith
Copy link
Member

Closing as discussed in #54372 (comment)

@oscardssmith oscardssmith removed the status:triage This should be discussed on a triage call label May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

String constructor on a view can mutate parent
8 participants