add `wrap` function which is the safe counterpart to `unsafe_wrap`. #52049

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.

Sign up for GitHub

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

Jump to bottom

Merged

vtjnash merged 16 commits into JuliaLang:master from MasonProtter:wrap

Dec 9, 2023

Contributor

MasonProtter commented Nov 6, 2023 •

edited

Loading

This gives users a way to create new Arrays out of existing Memory since Array has no constructors still. ~~There's a @test_broken in there due to #52048~~


          add wrap function which is the safe counterpart to unsafe_wrap.

018cfdf

Member

LilithHafner commented Nov 6, 2023 •

edited

Loading

~~This LGTM~~ except for the length issue which is definitely blocking. We can't have arrays floating around with incorrect lengths.

I wish we could call this function Array, but the existing Array "constructors" are copying and this isn't.

EDIT: yipes, I didn't realize these internals still had so many gotchas. Thank you for the review, @vtjnash.

vtjnash reviewed

View reviewed changes

base/array.jl Outdated Show resolved Hide resolved

MasonProtter mentioned this pull request

a::Array{T, N>1} assumes that length(a) = length(a.ref.mem) #52048

Closed

vtjnash reviewed

View reviewed changes

base/array.jl Outdated Show resolved Hide resolved

vtjnash reviewed

View reviewed changes

base/array.jl Outdated Show resolved Hide resolved


          update from review

e333739

MasonProtter commented

View reviewed changes

base/array.jl Outdated

    
                  mem = ref.mem

                  mem_len = length(mem)

                  len = Core.checked_dims(dims...)

                  @boundscheck mem_len >= len || invalid_wrap_err(men_len, dims)

Contributor Author

MasonProtter Nov 6, 2023 •

edited

Loading

should this check be behind a @boundscheck or should it just always be there?


          fix docs

75c177d

Seelengrab reviewed

View reviewed changes

base/array.jl Outdated Show resolved Hide resolved

MasonProtter added 2 commits

November 9, 2023 13:22


          fix typo

4cac28b


          Merge branch 'master' into wrap

2cdee6c

vtjnash mentioned this pull request

Avoid unalias() for matching Array{T} when broadcasting #52286

Closed

vtjnash reviewed

View reviewed changes

base/array.jl Outdated Show resolved Hide resolved

MasonProtter and others added 3 commits

November 26, 2023 22:20


          Update base/array.jl

3833df9

Co-authored-by: Jameson Nash <[email protected]>


          Update base/array.jl

30d387a

Co-authored-by: Jameson Nash <[email protected]>


          Update array.jl

3e85253

vtjnash reviewed

View reviewed changes

base/array.jl Outdated Show resolved Hide resolved

MasonProtter added 7 commits

November 26, 2023 22:25


          Update array.jl

0e70a6d


          test with an offset memref

60748b5


          Update arrayops.jl

3937b28


          fix memory size checks

aafc381


          test for MemoryRef offsets causing oob

8fe2ff1


          Merge branch 'master' into wrap

6d1cb68


          fix typo

94d1acd

Contributor Author

MasonProtter commented Nov 27, 2023

okay this should be ready now


          Include link to PR

3ddbcda

brenhinkeller added arrays feature merge me labels

Member

LilithHafner commented Dec 9, 2023

@vtjnash do you concur?

vtjnash merged commit 84cfe04 into JuliaLang:master

Member

vtjnash commented Dec 9, 2023

Should we consider adding support to view now too, that calls into this?

Contributor Author

MasonProtter commented Dec 9, 2023 •

edited

Loading

I'm not sure it'd really be that useful for view, since the call to jl_genericmemory_slice would induce an allocation 😞

That's why I wanted a way to wrap without re-allocating the memory slice as in #52048

Contributor Author

MasonProtter commented Dec 9, 2023

I guess actually we could do it for 1D views without allocations. But it's kinda unsatisfying to have it only be nice for 1D.

LilithHafner removed the merge me label

MasonProtter deleted the wrap branch

December 13, 2023 11:29

Liozou mentioned this pull request

Qualify wrap calls to avoid conflict on julia v1.11 carlobaldassi/ArgParse.jl#128

Merged

fatteneder mentioned this pull request

Add Base.wrap to docs #53342

Merged

fatteneder added a commit that referenced this pull request


          Add Base.wrap to docs (#53342)

2ca3753

I thought that's reasonable in the light that `view` was not
implemented:
#52049 (comment)

jariji mentioned this pull request

wrap is a very generic name to export #53552

Closed

tecosaur pushed a commit to tecosaur/julia that referenced this pull request


          Add Base.wrap to docs (JuliaLang#53342)

62767be

I thought that's reasonable in the light that `view` was not
implemented:
JuliaLang#52049 (comment)

KristofferC pushed a commit that referenced this pull request


          Revert "add wrap function which is the safe counterpart to `unsafe_…

d3b8ebf

…wrap`. (#52049)"

This reverts commit 84cfe04.

KristofferC mentioned this pull request

view(::Memory, ::UnitRange) should not create a Vector #54768

Closed

nsajko mentioned this pull request

[FR] make the wrap function public #58729

Open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels