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 firstindex in replace_ref_begin_end (fixes #41630) #41695

Merged
merged 5 commits into from
Aug 11, 2021

Conversation

phipsgabler
Copy link
Contributor

@phipsgabler phipsgabler commented Jul 24, 2021

I'd appreciate feedback about the tests. I have no idea how to use Revise on Base...

fix #41630

@simeonschaub simeonschaub added arrays [a, r, r, a, y, s] backport 1.6 Change should be backported to release-1.6 backport 1.7 bugfix This change fixes an existing bug labels Jul 24, 2021
@simeonschaub
Copy link
Member

simeonschaub commented Jul 24, 2021

I have no idea how to use Revise on Base...

You just need to build Julia once and when you run it, you can do Revise.track(Base) to pick up all changes inside base/. You can also run the tests with e.g. make test-revise-subarray to run them using Revise.

Copy link
Member

@simeonschaub simeonschaub left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me.

@jishnub
Copy link
Contributor

jishnub commented Jul 24, 2021

Instead of recreating offsetarrays in the test, would it make more sense to add the tests to test/offsetarray.jl which already uses such an implementation? This might make debugging easier

@simeonschaub
Copy link
Member

Yeah, didn't think of that, but that would be nice, so we don't have to duplicate this.

@phipsgabler
Copy link
Contributor Author

Uh, great, I didn't know that exists. So should I just move over the tests to that file? Or using .Main.OffsetArrays?

@jishnub
Copy link
Contributor

jishnub commented Jul 25, 2021

Perhaps moving the test to that file would make more sense, as OffsetArrays is definitely loaded at that point. Also because this fix is specific to OffsetArrays, and the indexing operation (accidentally) works correctly for standard 1-indexed arrays. In any case perhaps adding an extra test for standard arrays to test/subarray.jl might not be a bad idea.

@phipsgabler
Copy link
Contributor Author

I have moved the tests over to the other file. Something weird was going on with Github merging the upstream, I hope everthing is in good shape now...

Anyway, I'll be on vacation from tomorrow on, so if this is urgent, someone else needs to take care of it now.

@simeonschaub simeonschaub added the merge me PR is reviewed. Merge when all tests are passing label Aug 11, 2021
@simeonschaub simeonschaub merged commit 2ebbb2b into JuliaLang:master Aug 11, 2021
@simeonschaub
Copy link
Member

Thanks!

@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Aug 12, 2021
@phipsgabler phipsgabler deleted the phg/replace_ref_begin_end branch August 14, 2021 10:25
KristofferC pushed a commit that referenced this pull request Aug 25, 2021
KristofferC pushed a commit that referenced this pull request Aug 25, 2021
KristofferC pushed a commit that referenced this pull request Aug 31, 2021
KristofferC pushed a commit that referenced this pull request Sep 3, 2021
@KristofferC KristofferC removed backport 1.6 Change should be backported to release-1.6 backport 1.7 labels Sep 7, 2021
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
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] bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in @view: begin indexing vs. replace_ref_begin_end!
5 participants