-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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.
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
WIP: fix invoke for overloaded call #8862
Conversation
d82f1c9
to
733c774
Compare
If the first argument is a type it needs to be wrapped with Using |
733c774
to
ac8ba72
Compare
Okay, I added the Certainly, in all the applications that I can think of where you use
I was hoping that this would be a trivial patch, but if it starts getting hairy and brittle (because of the difficulty of determining the correct module), then it's probably not worth it. |
My reason for reporting this was that there's code in DataFrames that calls |
Isn't this really just the same problem that |
Yes, very similar. |
Could we play some trickery for a small set of these Core builtin functions and pass the function lambda's definition module as a "magic" parameter? I guess I don't really know how that would work with generic jl_apply, however. |
The only efficient way I can see to do that is what I did for |
That's perhaps not too terrible, although not particularly general. The alternative would seem to be to parse them differently, like ccall |
Isn't that worse? |
Is this still relevant or should it be closed? cc @yuyichao since some of your open PR's are in a similar area |
Actually I didn't know about this one... I think this is still relavant since this is a slightly different approach. |
This is probably no longer relevant since jb/functions? |
edit: pressed the wrong key... Yes, I think this is fixed by julia> type A
end
julia> (::A)(::Int) = 1
julia> (::A)(::Integer) = 2
julia> A()(1)
1
julia> invoke(A(), Tuple{Integer}, 1)
2 |
Should the test be committed first? |
Yeah, might be good to commit the test. |
This is for issue #8861.
However, something is not quite working ... the following error seems rather mysterious to me:(Update: this test now passes.)I'm guessing that there is a simple tweak to my patch that will fix this, but I don't understand where the problem lies.
cc: @simonster, @JeffBezanson, @vtjnash