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.

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

Add Base.wrap to docs #53342

Merged
merged 1 commit into from
Feb 17, 2024
Merged

Add Base.wrap to docs #53342

merged 1 commit into from
Feb 17, 2024

Conversation

fatteneder
Copy link
Member

@fatteneder fatteneder commented Feb 15, 2024 •
edited
Loading

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

@fatteneder fatteneder added the docs This change adds or pertains to documentation label Feb 15, 2024
@fatteneder fatteneder requested a review from vtjnash February 15, 2024 14:45
@Tokazama
Copy link
Contributor

Is wrap supposed to be a generic function for wrapping data in other structs?

@fatteneder
Copy link
Member Author

Its meant to wrap an Array around a Memory:

julia/base/array.jl

Lines 3066 to 3072 in e460d35

"""
wrap(Array, m::Union{Memory{T}, MemoryRef{T}}, dims)
Create an array of size `dims` using `m` as the underlying memory. This can be thought of as a safe version
of [`unsafe_wrap`](@ref) utilizing `Memory` or `MemoryRef` instead of raw pointers.
"""
function wrap end

@Tokazama
Copy link
Contributor

Its meant to wrap an Array around a Memory:

julia/base/array.jl

Lines 3066 to 3072 in e460d35

"""
wrap(Array, m::Union{Memory{T}, MemoryRef{T}}, dims)
Create an array of size `dims` using `m` as the underlying memory. This can be thought of as a safe version
of [`unsafe_wrap`](@ref) utilizing `Memory` or `MemoryRef` instead of raw pointers.
"""
function wrap end

But is it intended to be a method that is extended for similar use cases?

@fatteneder
Copy link
Member Author

I think not, because

  • it was added as a safe counterpart to unsafe_wrap, which itself is only meant to wrap an Array around a Ptr,
  • the arguments are specific to Array (what would the dims argument be use for with a generic struct?),
  • I think there is no functionality yet that builds upon wrap, so no need to extend it.

@KristofferC KristofferC added the backport 1.11 Change should be backported to release-1.11 label Feb 16, 2024
@MasonProtter
Copy link
Contributor

Oops, I forgot to add this to the docs when I wrote it. Thanks for putting it in!

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Feb 17, 2024
@vtjnash
Copy link
Member

vtjnash commented Feb 17, 2024

The generic version is generally view, though that usually returns an immutable-axes container, while this is fairly specific to making a mutable Array wrapper around Memory. So someone else might extend this, but there is not too many envisioned cases where there is a similar set of factors. It probably only would be extended for other near-equivalent cases, like converting some struct that wrapped a Memory into a type that wraps an Array, or doing something similar with AtomicMemory

@fatteneder fatteneder merged commit 2ca3753 into JuliaLang:master Feb 17, 2024
12 checks passed
@fatteneder fatteneder removed the merge me PR is reviewed. Merge when all tests are passing label Feb 17, 2024
@fatteneder fatteneder deleted the fa/wrap branch February 17, 2024 20:19
@KristofferC KristofferC mentioned this pull request Feb 26, 2024
Merged
28 tasks
KristofferC added a commit that referenced this pull request Mar 1, 2024
@KristofferC
Backports release 1.11 (#53472)
2911937 
Backported PRs:
- [x] #53361 <!-- 🤖 [master] Bump the SparseArrays stdlib from c9f7293
to cb602d7 -->
- [x] #53300 <!-- allow external absint to hold custom data in
`codeinst.inferred` -->
- [x] #53342 <!-- Add `Base.wrap` to docs -->
- [x] #53372 <!-- Silence warnings in `test/file.jl` -->
- [x] #53357 <!-- 🤖 [master] Bump the Pkg stdlib from 6dd0e7c9e to
76070d295 -->
- [x] #53373 <!-- fix sysimage-native-code=no option with pkgimages -->
- [x] #53333 <!-- More consistent return value for annotations, take 2
-->
- [x] #53354 <!-- fix code coverage bug in tail position and `else` -->
- [x] #53407 <!-- fix sysimage-native-code=yes option -->
- [x] #53388 <!-- Fix documentation: thread pool of main thread -->
- [x] #53355 <!-- Fix synchronization issues on the GC scheduler. -->
- [x] #53429 <!-- Subtype: skip slow-path in `local_∀_∃_subtype` if
inputs contain no ∃ typevar. -->
- [x] #53437 <!-- Add debug variant of loader_trampolines.o -->
- [x] #53284 <!-- Add annotate! method for AnnotatedIOBuffer -->
- [x] #53466 <!-- [MozillaCACerts_jll] Update to v2023-12-12 -->
- [x] #53467 <!-- [LibGit2_jll] Update to v1.7.2 -->
- [x] #53326 <!-- RFC: when loading code for internal purposes, load
stdlib files directly, bypassing DEPOT_PATH, LOAD_PATH, and stale checks
-->
- [x] #53332
- [x] #53320 <!-- Add `Sys.isreadable, Sys.iswriteable`, update `ispath`
-->
- [x] #53476

Contains multiple commits, manual intervention needed:
- [ ] #53285 <!-- Add update mechanism for Terminfo, and common
user-alias data -->

Non-merged PRs with backport label:
- [ ] #53424 <!-- yet more atomics & cache-line fixes on work-stealing
queue -->
- [ ] #53408 <!-- task splitting: change additive accumulation to
multiplicative -->
- [ ] #53403 <!-- Move parallel precompilation to Base -->
- [ ] #53402 <!-- Add `jl_getaffinity` and `jl_setaffinity` -->
- [ ] #53391 <!-- Default to the medium code model in x86 linux -->
- [ ] #53125 <!-- coverage: count coverage where explicitly requested by
inference only -->
- [ ] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Mar 1, 2024
tecosaur pushed a commit to tecosaur/julia that referenced this pull request Mar 4, 2024
@fatteneder @tecosaur
Add Base.wrap to docs (JuliaLang#53342)
62767be 
I thought that's reasonable in the light that `view` was not
implemented:
JuliaLang#52049 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash Awaiting requested review from vtjnash

Assignees
No one assigned
Labels
docs This change adds or pertains to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fatteneder @Tokazama @MasonProtter @vtjnash @KristofferC