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 #39379, use first∘LinearIndices instead of 1 #39393

Merged
merged 2 commits into from
Jan 28, 2021

Conversation

mbauman
Copy link
Sponsor Member

@mbauman mbauman commented Jan 25, 2021

to resolve a zero-index getindex to a linear index.

fix #39379

to resolve a zero-index getindex to a linear index.
@mbauman mbauman added arrays [a, r, r, a, y, s] bugfix This change fixes an existing bug backport 1.6 Change should be backported to release-1.6 labels Jan 25, 2021
@mbauman
Copy link
Sponsor Member Author

mbauman commented Jan 25, 2021

Note that this is a zero-cost abstraction:

julia> @code_llvm Base._to_linear_index([])
;  @ abstractarray.jl:1196 within `_to_linear_index'
define i64 @julia__to_linear_index_133({}* nonnull align 16 dereferenceable(40) %0) {
top:
  ret i64 1
}

test/offsetarray.jl Outdated Show resolved Hide resolved
@simeonschaub
Copy link
Member

Is it a problem that _to_linear_index will now throw for empty arrays? It might at least make some error messages potentially more confusing.

@simeonschaub
Copy link
Member

Ah, never mind. We do actually support first for empty ranges.

@mbauman
Copy link
Sponsor Member Author

mbauman commented Jan 25, 2021

Is it a problem that _to_linear_index will now throw for empty arrays?

It doesn't (see the unconditional ret 1 above).

julia> LinearIndices([])
0-element LinearIndices{1, Tuple{Base.OneTo{Int64}}}

julia> first(LinearIndices([]))
1

@KristofferC
Copy link
Sponsor Member

@nanosoldier runbenchmarks("array", vs = ":master")

@nanosoldier
Copy link
Collaborator

Something went wrong when running your job:

NanosoldierError: error when preparing/pushing to report repo: failed process: Process(`git pull -X ours`, ProcessExited(1)) [1]

Unfortunately, the logs could not be uploaded.
cc @christopher-dG

@christopher-dG
Copy link
Member

I upgraded nanosoldier.jl on one of the nodes, I'll have a look at this in the morning

@KristofferC KristofferC mentioned this pull request Jan 26, 2021
60 tasks
@mbauman
Copy link
Sponsor Member Author

mbauman commented Jan 26, 2021

I’d be surprised if we even have any benchmarks for A[] where A is not zero-dimensional.

@KristofferC
Copy link
Sponsor Member

I just wanted to check that there wasn't any overhead from this in normal benchmarks (things starting to fail to inline etc).

@JeffBezanson JeffBezanson merged commit 9de107a into JuliaLang:master Jan 28, 2021
KristofferC pushed a commit that referenced this pull request Jan 29, 2021
@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Feb 1, 2021
KristofferC pushed a commit that referenced this pull request Feb 1, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
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.

zero-argument getindex method in Base incorrectly assumes 1-based indexing
6 participants