Skip to content

Conversation

@mbauman
Copy link
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
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
}

@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
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
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
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
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