Make reshape and view on Memory produce Arrays and delete wrap #53896
Conversation
…ave reshape(view(::Memory, ...), ...)
|
The |
|
I dropped the Should we support slices? view(m::GenericMemory{M, T}, inds::Colon) where {M, T} = view(m, eachindex(m)) |
I think |
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
|
IIUC it is possible for resizing a vector produced here to corrupt the underlying memory. If I'm correct, that should be documented. |
|
Seemingly unrelated CI failure, but master is green. Updating branch. |
|
The failure, |
|
I think it might be an issue if the compiler isn't able to infer the type of taking a view of a Memory. |
|
The compiler knows that just fine julia> Base.return_types(view, (Memory{Int}, UnitRange))
1-element Vector{Any}:
Vector{Int64} (alias for Array{Int64, 1})It's the return type of UnitRange when given a non-Int integer that is Any julia> Base.return_types(:, (Int, Integer))
2-element Vector{Any}:
UnitRange{Int64}
Any |
|
I'm ready to merge if @vtjnash and @oscardssmith think it's ready. |
|
Nothing complex or controversial has changed since @vtjnash's approval. I'm going to go ahead and merge. |
| @@ -42,7 +42,7 @@ end | |||
|
|
|||
| # allocate Vector{UInt8}s for IOBuffer storage that can efficiently become Strings | |||
| StringMemory(n::Integer) = unsafe_wrap(Memory{UInt8}, _string_n(n)) | |||
| StringVector(n::Integer) = wrap(Array, StringMemory(n)) | |||
| StringVector(n::Integer) = view(StringMemory(n), 1:n)::Vector{UInt8} | |||
There was a problem hiding this comment.
I think this change broke JSON3:
(base) oscar@Oscars-MBP JSONSchema % julia +nightly
_
_ _ _(_)_ | Documentation: https://docs.julialang.org
(_) | (_) (_) |
_ _ _| |_ __ _ | Type "?" for help, "]?" for Pkg help.
| | | | | | |/ _` | |
| | |_| | | | (_| | | Version 1.12.0-DEV.313 (2024-04-07)
_/ |\__'_|_|_|\__'_| | Commit c5a3b653353 (0 days old master)
|__/ |
julia> Base.StringVector(UInt64(2))
ERROR: TypeError: in new, expected Tuple{Int64}, got a value of type Tuple{UInt64}
Stacktrace:
[1] view
@ ./genericmemory.jl:312 [inlined]
[2] StringVector(n::UInt64)
@ Base ./iobuffer.jl:45
[3] top-level scope
@ REPL[1]:1
julia> exit()
(base) oscar@Oscars-MBP JSONSchema % julia
_
_ _ _(_)_ | Documentation: https://docs.julialang.org
(_) | (_) (_) |
_ _ _| |_ __ _ | Type "?" for help, "]?" for Pkg help.
| | | | | | |/ _` | |
| | |_| | | | (_| | | Version 1.10.2 (2024-03-01)
_/ |\__'_|_|_|\__'_| | Official https://julialang.org/ release
|__/ |
julia> Base.StringVector(UInt64(2))
2-element Vector{UInt8}:
0x50
0x47Example: https://github.com/quinnj/JSON3.jl/blob/6429adafd02a7ad07fa80e9a0ad21b098635244c/src/write.jl#L32
MathOptInterface failing on nightly: https://github.com/jump-dev/MathOptInterface.jl/actions/runs/8591539106/job/23540421403?pr=2464
There was a problem hiding this comment.
Thank you for reporting. I can confirm that this is a bug and this PR introduced it.
There was a problem hiding this comment.
PR fixing this: #53991
|
there is also a bug on CSV.jl that seems to be activated with this PR, but i suppose that is for the same reasons as the last bug (CSV really wants a |
Can you elaborate? Reproducible example and stack trace? |
The main problem is that |
- Make reshape and view with one based indexing on Memory produce Arrays - delete wrap Implements #53552 (comment) --------- Co-authored-by: Jameson Nash <vtjnash@gmail.com> (cherry picked from commit 273d91e)
Backported PRs: - [x] #53757 <!-- Add an IndexStyle example to the diagind docstring --> - [x] #53809 <!-- Add missing GC_POP() in emit_cfunction --> - [x] #53789 <!-- also check that UUID of project is non-null when treating it as a package --> - [x] #53805 <!-- precompilepkgs: simplify custom config printing if only one --> - [x] #53822 <!-- Bump libuv --> - [x] #53837 <!-- update MPFR to 4.2.1 --> - [x] #53862 <!-- precompilepkgs: fix error reporting --> - [x] #53774 <!-- Remove some duplicates from emitted compilation traces --> - [ ] #53696 <!-- add invokelatest to on_done callback in bracketed paste --> - [x] #53383 <!-- Add `_unsetindex!` methods for `SubArray`s and `CartesianIndex`es --> - [x] #53475 <!-- Fix boundscheck in unsetindex for SubArrays --> - [x] #53888 - [x] #53870 <!-- Revert change to checksum for llvm-julia --> - [x] #53906 <!-- Add `Base.isrelocatable(pkg)` --> - [x] #53833 <!-- Profile: make heap snapshots viewable in vscode viewer --> - [x] #53961 <!-- `LazyString` in `LinearAlgebra.checksquare` error message --> - [x] #53962 <!-- Use StringMemory instead of StringVector where possible --> - [x] #53825 <!-- profile: doc: update the `Allocs.@profile` doc string --> - [x] #53975 <!-- `LazyString` in `DimensionMismatch` error messages in broadcasting --> - [x] #53905 <!-- Avoid repeated precompilation when loading from non-relocatable cachefiles --> - [x] #53896 <!-- Make reshape and view on Memory produce Arrays and delete wrap --> - [x] #53991 <!-- Test and fix non-int-length bug in `view(::Memory, ::Union{UnitRange, Base.OneTo})` -->
* fix breakage caused by JuliaLang/julia/pull/53896 * make __wrap compatible with 1.11 RC
KristofferC
removed
the
backport 1.11
Make reshape and (one-based) view on Memory produce Arrays and delete wrap
Implements #53552 (comment)
Fixes #53552