Skip to content

Rename Memory wrapper from view to unsafe_vector#54273

Closed
nhz2 wants to merge 2 commits intoJuliaLang:masterfrom
nhz2:nz/memory-2-vector-rename
Closed

Rename Memory wrapper from view to unsafe_vector#54273
nhz2 wants to merge 2 commits intoJuliaLang:masterfrom
nhz2:nz/memory-2-vector-rename

Conversation

@nhz2
Copy link
Copy Markdown
Member

@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 triage This should be discussed on a triage call label Apr 26, 2024
@christiangnrd
Copy link
Copy Markdown
Contributor

christiangnrd commented Apr 26, 2024

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

@vtjnash
Copy link
Copy Markdown
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
Copy Markdown
Member

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
Copy Markdown
Member 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
Copy Markdown
Contributor

longemen3000 commented Apr 26, 2024 •
edited
Loading

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
Copy Markdown
Contributor

jariji commented Apr 26, 2024 •
edited
Loading

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
Copy Markdown
Member

nsajko commented Apr 28, 2024

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

@jariji
Copy link
Copy Markdown
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 oscardssmith mentioned this pull request May 9, 2024
Merged
@oscardssmith
Copy link
Copy Markdown
Member

oscardssmith commented May 9, 2024

Closing as discussed in #54372 (comment)

@oscardssmith oscardssmith removed the 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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

String constructor on a view can mutate parent

8 participants

@nhz2 @christiangnrd @vtjnash @adienes @longemen3000 @jariji @nsajko @oscardssmith