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

Fix push! for OffsetVectors, add tests for push! and append! on AbstractVector #55480

Merged
merged 2 commits into from
Aug 16, 2024

Conversation

mkitti
Copy link
Contributor

@mkitti mkitti commented Aug 13, 2024

Per #55470 (comment), the push!(::AbstractArray, ...) array implementation assumed one-based indexing and did not account for an OffsetVector
scenario.

Here we add tests for push!(::AbstractArray, ...) and append(::AbstractArray, ...) including using @invoke to test the effect on OffsetVector.

cc: @fredrikekre

@mkitti
Copy link
Contributor Author

mkitti commented Aug 13, 2024

I do not understand the CI failures here. They seem unrelated to the code changes.

@nsajko nsajko added the arrays [a, r, r, a, y, s] label Aug 13, 2024
@jishnub
Copy link
Contributor

jishnub commented Aug 14, 2024

Looks good to me

args in (("eenie",), ("eenie", "minie"), ("eenie", "minie", "mo"))
orig = copy(a)
push!(a, args...)
@test length(a) == length(orig) + length(args)
@test a[axes(orig,1)] == orig
@test all(a[end-length(args)+1:end] .== args)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this test equivalent to the simpler a[end-length(args)+1:end] == args?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It should be.

@jishnub jishnub merged commit 5230d27 into JuliaLang:master Aug 16, 2024
7 of 9 checks passed
@fredrikekre
Copy link
Member

Please don't include any @-mentions in commit messages. Now I am pinged whenever 5230d27 is pushed.

@jishnub
Copy link
Contributor

jishnub commented Aug 16, 2024

Oops, sorry

lazarusA pushed a commit to lazarusA/julia that referenced this pull request Aug 17, 2024
…actVector (JuliaLang#55480)

Per
JuliaLang#55470 (comment),
the `push!(::AbstractArray, ...)` array implementation assumed one-based
indexing and did not account for an `OffsetVector`
scenario.

Here we add tests for `push!(::AbstractArray, ...)` and
`append(::AbstractArray, ...)` including using `@invoke` to test the
effect on `OffsetVector`.

cc: @fredrikekre
@tomaklutfu
Copy link
Contributor

Shouldn't this also be backported to julia 1.11 as a follow up the previous PR?

@jishnub jishnub added the backport 1.11 Change should be backported to release-1.11 label Aug 26, 2024
KristofferC pushed a commit that referenced this pull request Sep 9, 2024
…actVector (#55480)

Per
#55470 (comment),
the `push!(::AbstractArray, ...)` array implementation assumed one-based
indexing and did not account for an `OffsetVector`
scenario.

Here we add tests for `push!(::AbstractArray, ...)` and
`append(::AbstractArray, ...)` including using `@invoke` to test the
effect on `OffsetVector`.

cc: @fredrikekre
(cherry picked from commit 5230d27)
@KristofferC KristofferC mentioned this pull request Sep 11, 2024
33 tasks
KristofferC pushed a commit that referenced this pull request Sep 12, 2024
…actVector (#55480)

Per
#55470 (comment),
the `push!(::AbstractArray, ...)` array implementation assumed one-based
indexing and did not account for an `OffsetVector`
scenario.

Here we add tests for `push!(::AbstractArray, ...)` and
`append(::AbstractArray, ...)` including using `@invoke` to test the
effect on `OffsetVector`.

cc: @fredrikekre
KristofferC added a commit that referenced this pull request Sep 17, 2024
Backported PRs:
- [x] #55480 <!-- Fix push! for OffsetVectors, add tests for push! and
append! on AbstractVector -->
- [x] #55443 <!-- Add test for upper/lower/titlecase and fix call -->
- [x] #55524 <!-- Set `.jl` sources as read-only during installation -->
- [x] #55500 <!-- make jl_thread_suspend_and_get_state safe -->
- [x] #55506 <!-- Fix indexing in _mapreducedim for OffsetArrays -->
- [x] #55564 <!-- Empty out loaded_precompiles dict instead of asserting
it's empty. -->
- [x] #55567 <!-- Initialize threadpools correctly during sysimg build
-->
- [x] #55596 <!-- Fast bounds-check for CartesianIndex ranges -->
- [x] #55605 <!-- Reroute Symmetric/Hermitian + Diagonal through
triangular -->
- [x] #55640 <!-- win: move stack_overflow_warning to the backtrace
fiber -->
- [x] #55715 <!-- Add precompile signatures to Markdown to reduce
latency. -->
- [x] #55593 <!-- Fix invalidations for FileIO -->
- [x] #55555 <!-- Revert "Don't expose guard pages to malloc_stack API
consumers" -->
- [x] #55720 <!-- Fix `pkgdir` for extensions -->
- [x] #55729 <!-- Avoid confounding compilation side effects of
`@time_imports` -->
- [x] #55718 <!-- Fix `@time_imports` extension recognition -->
- [x] #55522 <!-- Fix tr for Symmetric/Hermitian block matrices -->

Contains multiple commits, manual intervention needed:
- [ ] #55509 <!-- Fix cong implementation to be properly random and not
just cycling. -->

Non-merged PRs with backport label:
- [ ] #55641 <!-- fall back to slower stat filesize if optimized
filesize fails -->
- [ ] #55534 <!-- Set stdlib sources as read-only during installation
-->
- [ ] #55499 <!-- propagate the terminal's `displaysize` to the
`IOContext` used by the REPL -->
- [ ] #55458 <!-- Allow for generically extracting unannotated string
-->
- [ ] #55457 <!-- Make AnnotateChar equality consider annotations -->
- [ ] #55453 <!-- Privatise the annotations API, for StyledStrings -->
- [ ] #55355 <!-- relocation: account for trailing path separator in
depot paths -->
- [ ] #55220 <!-- `isfile_casesensitive` fixes on Windows -->
- [ ] #55169 <!-- `propertynames` for SVD respects private argument -->
- [ ] #54457 <!-- Make `String(::Memory)` copy -->
- [ ] #53957 <!-- tweak how filtering is done for what packages should
be precompiled -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
- [ ] #50813 <!-- More doctests for Sockets and capitalization fix -->
- [ ] #50157 <!-- improve docs for `@inbounds` and
`Base.@propagate_inbounds` -->
- [ ] #41244 <!-- Fix shell `cd` error when working dir has been deleted
-->
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants