safer indexing for AbstractArray #540
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #540 +/- ##
==========================================
+ Coverage 80.65% 80.66% +0.01%
==========================================
Files 26 26
Lines 1701 1702 +1
==========================================
+ Hits 1372 1373 +1
Misses 329 329 ☔ View full report in Codecov by Sentry. |
| for i in 1:length(a) | ||
| ra[i] = a[i] | ||
| for (i, idx) in enumerate(eachindex(a)) | ||
| ra[i] = a[idx] |
There was a problem hiding this comment.
Note that this still assumes 1-based indexing for ra since enumerate will start with 1.
There was a problem hiding this comment.
yes, but we create ra and it's indeed one based
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
typeof(ra) is lacking a eachindex method and the fallback calls keys; I'm going to take a stab at defining something appopriate
Looks like something is getting |
| @@ -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)) | |||
There was a problem hiding this comment.
Feels weird to define eachindex for a Ptr but we also have iterate and getindex...
There was a problem hiding this comment.
yeah, it does, but then again we're treating R native arrays as iterators without copying...
| # 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] |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Hmmm, yeah, but I'm going to punt on that until I've tackled #444
closes # 431apparently not on Mac