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.

Sign up for GitHub

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

safer indexing for AbstractArray #540

Merged
merged 4 commits into from
Jul 24, 2024
Merged

safer indexing for AbstractArray #540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f7aa5f3
safer indexing for AbstractArray
palday Jul 19, 2024
d2d815c
Merge branch 'master' into pa/eachindex
palday Jul 19, 2024
a7c44c3
more eachindex
palday Jul 21, 2024
31626b6
eachindex method
palday Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/convert/axisarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
dnames = getattrib(r, Const.DimNamesSymbol)
isnull(dnames) && error("r has no dimnames")
dsym = rcopy(Array{Symbol}, getnames(dnames))
for i in 1:length(dsym)
for i in eachindex(dsym)

Check warning on line 7 in src/convert/axisarray.jl

View check run for this annotation

Codecov / codecov/patch

src/convert/axisarray.jl#L7 

Added line #L7 was not covered by tests
if dsym[i] == Symbol("")
dsym[i] = i == 1 ? (:row) : i == 2 ? (:col) : i == 3 ? (:page) : Symbol(:dim_, i)
end
Expand Down
20 changes: 12 additions & 8 deletions src/convert/base.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,15 +114,15 @@ end
function rcopy(::Type{Vector{Bool}},s::Ptr{LglSxp})
a = Array{Bool}(undef, length(s))
v = unsafe_vec(s)
for i = 1:length(a)
for i in eachindex(a, v)
a[i] = v[i] != 0
end
a
end
function rcopy(::Type{BitVector},s::Ptr{LglSxp})
a = BitArray(undef, length(s))
v = unsafe_vec(s)
for i = 1:length(a)
for i in eachindex(a, v)
a[i] = v[i] != 0
end
a
Expand All @@ -135,15 +135,15 @@ end
function rcopy(::Type{Array{Bool}},s::Ptr{LglSxp})
a = Array{Bool}(undef, size(s)...)
v = unsafe_vec(s)
for i = 1:length(a)
for i in eachindex(a, v)
a[i] = v[i] != 0
end
a
end
function rcopy(::Type{BitArray},s::Ptr{LglSxp})
a = BitArray(undef, size(s)...)
v = unsafe_vec(s)
for i = 1:length(a)
for i in eachindex(a, v)
a[i] = v[i] != 0
end
a
Expand Down Expand Up @@ -264,8 +264,10 @@ sexp(::Type{RClass{:character}},st::AbstractString) = sexp(RClass{:character}, s
function sexp(::Type{RClass{:character}}, a::AbstractArray{T}) where T<:AbstractString
ra = protect(allocArray(StrSxp, size(a)...))
try
for i in 1:length(a)
ra[i] = a[i]
# we want this to work even if a doesn't use one-based indexing
# we only care about ra having the same length (which it does)
for (i, idx) in zip(eachindex(ra), eachindex(a))
ra[i] = a[idx]
Copy link
Member

Choose a reason for hiding this comment

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

Note that this still assumes 1-based indexing for ra since enumerate will start with 1.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, but we create ra and it's indeed one based

Copy link
Member

Choose a reason for hiding this comment

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

Fair, though we also create a and v as 1-based arrays in the above rcopy methods which means the loop specifications there were safe prior to using eachindex. I assumed you were going for minimal assumptions but if not then that's fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

typeof(ra) is lacking a eachindex method and the fallback calls keys; I'm going to take a stab at defining something appopriate

end
finally
unprotect(1)
Expand Down Expand Up @@ -296,8 +298,10 @@ end
function sexp(::Type{RClass{:list}}, a::AbstractArray)
ra = protect(allocArray(VecSxp, size(a)...))
try
for i in 1:length(a)
ra[i] = a[i]
# we want this to work even if a doesn't use one-based indexing
# we only care about ra having the same length (which it does)
for (i, idx) in zip(eachindex(ra), eachindex(a))
ra[i] = a[idx]
Copy link
Member

Choose a reason for hiding this comment

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

This feels like a situation where a copyto! method could make sense, assuming such a method doesn't already exist (I assume it doesn't)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm, yeah, but I'm going to punt on that until I've tackled #444

end
finally
unprotect(1)
Expand Down
1 change: 1 addition & 0 deletions src/methods.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ function iterate(s::Ptr{S}, state) where S<:VectorSxp
(s[state], state)
end

Base.eachindex(s::Ptr{<:VectorSxp}) = Base.OneTo(length(s))
Copy link
Member

Choose a reason for hiding this comment

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

Feels weird to define eachindex for a Ptr but we also have iterate and getindex...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, it does, but then again we're treating R native arrays as iterators without copying...


"""
Set element of a VectorSxp by a label.
Expand Down
Loading