Skip to content

Commit

Permalink
disentangled arrays in iteration: repl, serialization and shared arrays
Browse files Browse the repository at this point in the history
  • Loading branch information
@dfdx
dfdx committed Mar 11, 2016
1 parent 3820cf3 commit 9491dea
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 16 deletions.
2 changes: 1 addition & 1 deletion base/REPLCompletions.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ function complete_symbol(sym, ffunc)
# We're now looking for a type
fields = fieldnames(t)
found = false
for i in 1:length(fields)
for i in eachindex(fields)
s == fields[i] || continue
t = t.types[i]
(Base.isstructtype(t) && !(t <: Tuple)) || return UTF8String[]
Expand Down
2 changes: 1 addition & 1 deletion base/replutil.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,7 @@ function process_backtrace(process_func::Function, top_function::Symbol, t::Vect
n = 0
last_frame = StackTraces.UNKNOWN
count = 0
for i = 1:length(t)
for i = eachindex(t)
lkup = StackTraces.lookup(t[i])
if lkup === StackTraces.UNKNOWN
continue
Expand Down
18 changes: 8 additions & 10 deletions base/serialize.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,10 @@ const TAGS = Any[

const ser_version = 3 # do not make changes without bumping the version #!

const NTAGS = length(TAGS)

function sertag(v::ANY)
ptr = pointer_from_objref(v)
ptags = convert(Ptr{Ptr{Void}}, pointer(TAGS))
@inbounds for i = 1:NTAGS
@inbounds for i in eachindex(TAGS)
ptr == unsafe_load(ptags,i) && return (i+1)%Int32

This comment has been minimized.

Sign in to view

Copy link
@nalimilan

nalimilan Mar 14, 2016

Member

Ugh. This doesn't sound correct at all. This only passed @timholy's review because he's on holiday! When I think I was bikeshedding with Tony about variable's when this happened... :-)

@dfdx Don't worry about this. Though for your next PR, note that when there are things like unsafe_* or conversion to Int*, better add a FIXME and ask, as these are unlikely to work with non-integer indices. Here, unsafe_load only accepts an integer as second argument.

I've made a PR at #15498.

This comment has been minimized.

Sign in to view

Copy link
@dfdx

dfdx Mar 14, 2016

Author Contributor

@nalimilan Actually, I thought more about other iterators that may be index by integer, but not efficiently (e.g. Dict with int keys) or not in range 1:length(A) (e.g. OffsetArrays). For this particular example I kept in mind a kind of IndexedSet (i.e. a Set that supports getindex and eachindex). This would allow both - efficient iteration (for every tag...) and lookup (is this keyword a tag?). This is a little bit contrived, but hopefully you've got the idea.

Does it make sense (both - in this specific case and in general) or for i=1:length(A) should be preferred whenever i is provably an integer?

This comment has been minimized.

Sign in to view

Copy link
@nalimilan

nalimilan Mar 14, 2016

Member

The question is not really whether it's provably an integer, it's rather whether it's used in places which need an integer argument. In that case, using eachindex doesn't really make sense. When you encounter such cases, it's best to leave a comment if the code is not very clear (i.e. when it's not obvious that we only expect iterating over a standard Array or tuple), and leave the code as-is when the code is clear.

Here TAGS is a global Array, so there's no problem in practice.

This comment has been minimized.

Sign in to view

Copy link
@timholy

timholy Mar 15, 2016

Member

Good catch @nalimilan. Indeed I should avoid merging PRs while on vacation. Thanks for stepping in!
end
return Int32(-1)
Expand Down Expand Up @@ -116,16 +114,16 @@ function serialize(s::SerializationState, t::Tuple)
writetag(s.io, LONGTUPLE_TAG)
write(s.io, Int32(l))
end
for i = 1:l
serialize(s, t[i])
for e in t
serialize(s, e)
end
end

function serialize(s::SerializationState, v::SimpleVector)
writetag(s.io, SIMPLEVECTOR_TAG)
write(s.io, Int32(length(v)))
for i = 1:length(v)
serialize(s, v[i])
for e in v
serialize(s, e)
end
end

Expand Down Expand Up @@ -195,7 +193,7 @@ function serialize(s::SerializationState, a::Array)
if isbits(elty)
serialize_array_data(s.io, a)
else
for i = 1:length(a)
for i in eachindex(a)

This comment has been minimized.

Sign in to view

Copy link
@nalimilan

nalimilan Mar 14, 2016

Member

Same here: isdefined only accepts integer indices currently. Though since a is restricted to be an Array, this is not an issue, and either version is fine.
if isdefined(a, i)
serialize(s, a[i])
else
Expand Down Expand Up @@ -260,7 +258,7 @@ function serialize(s::SerializationState, ex::Expr)
end
serialize(s, ex.head)
serialize(s, ex.typ)
for a = ex.args
for a in ex.args
serialize(s, a)
end
end
Expand Down Expand Up @@ -615,7 +613,7 @@ function deserialize_array(s::SerializationState)
end
A = Array(elty, dims)
deserialize_cycle(s, A)
for i = 1:length(A)
for i = eachindex(A)
tag = Int32(read(s.io, UInt8)::UInt8)
if tag != UNDEFREF_TAG
A[i] = handle_deserialize(s, tag)
Expand Down
8 changes: 4 additions & 4 deletions base/sharedarray.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ function SharedArray(T::Type, dims::NTuple; init=false, pids=Int[])
end

# Wait till all the workers have mapped the segment
for i in 1:length(refs)
wait(refs[i])
for ref in refs
wait(ref)
end

# All good, immediately unlink the segment.
Expand Down Expand Up @@ -183,8 +183,8 @@ function SharedArray{T,N}(filename::AbstractString, ::Type{T}, dims::NTuple{N,In
end

# Wait till all the workers have mapped the segment
for i in 1:length(refs)
wait(refs[i])
for ref in refs
wait(ref)
end

S = SharedArray{T,N}(dims, pids, refs, filename)
Expand Down

0 comments on commit 9491dea

Please sign in to comment.