Skip to content

Commit

Permalink
patch #3167 and #3400, by fixing _methods() to return only methods th…
Browse files Browse the repository at this point in the history
…at may actually get called for the argument tuple
  • Loading branch information
vtjnash committed Aug 20, 2013
1 parent c07c491 commit 847a6fa
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 1 deletion.
26 changes: 25 additions & 1 deletion base/reflection.jl
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,31 @@ function_name(f::Function) = isgeneric(f) ? f.env.name : (:anonymous)

code_lowered(f::Function,t::Tuple) = map(m->uncompressed_ast(m.func.code), methods(f,t))
methods(f::ANY,t::ANY) = map(m->m[3], _methods(f,t,-1))::Array{Any,1}
_methods(f::ANY,t::ANY,lim) = ccall(:jl_matching_methods, Any, (Any,Any,Int32), f, t, lim)
_methods(f::ANY,t::ANY,lim) = _methods(f,{(t::Tuple)...},length(t::Tuple),lim,{})
function _methods(f::ANY,t::Array,i,lim::Integer,matching::Array{Any,1})
if i == 0
new = ccall(:jl_matching_methods, Any, (Any,Any,Int32), f, tuple(t...), lim)
if new === false
return false
end
append!(matching, new::Array{Any,1})
else
ti = t[i]
if isa(ti, UnionType)
for ty in (ti::UnionType).types
t[i] = ty
if _methods(f,t,i-1,lim,matching) === false
t[i] = ty
return false
end
end
t[i] = ti
else
return _methods(f,t,i-1,lim,matching)
end
end
matching
end

function methods(f::Function)
if !isgeneric(f)
Expand Down
14 changes: 14 additions & 0 deletions test/core.jl
Original file line number Diff line number Diff line change
Expand Up @@ -916,3 +916,17 @@ end
@test isa(foo4075(Foo4075(int64(1),2.0),:y), Float64)
# very likely to segfault the second time if this is broken
@test isa(foo4075(Foo4075(int64(1),2.0),:y), Float64)

# issue #3167
function foo(x)
ret=Array(typeof(x[1]), length(x))
for j = 1:length(x)
ret[j] = x[j]
end
return ret
end
x = Array(Union(Dict{Int64,String},Array{Int64,3},Number,String,Nothing), 3)
x[1] = 1.0
x[2] = 2.0
x[3] = 3.0
foo(x) == [1.0, 2.0, 3.0]

3 comments on commit 847a6fa

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

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

REALLY not sure about this. I find that this does indeed prevent inlining in some cases by returning >1 item from _methods when only 1 method matches. Affects the cons perf test most of all.

Maybe a better implementation is to merge all results for a single method by unioning the types and environments together (nontrivial, but not too bad).

@vtjnash
Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer correctness over speed for the upcoming release.

It could be good to union the environments, but that all becomes irrelevant if my union types call specialization inlining is merged. It would still be useful then (to generate fewer call sites), but might require a different approach to using the information.

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

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

The concept here is extremely simple. You have inferred argument types that encompass all possible run--time types. This is intersected with a method signature, and you get a type back. There is no bug in that concept.

Please sign in to comment.