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.

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

RFC: Use invokelatest in jl_Function_call #416

Merged
merged 1 commit into from
Jul 28, 2017

Conversation

ihnorton
Copy link
Member

@ihnorton ihnorton commented Jul 22, 2017

Fixes JuliaPy/pyjulia#105

I thought it would also fix #343, but the example in that issue description seems to work fine for me on 0.6 release without this PR (but the issue is still open).

Directly calls Core._apply_latest with a closure over kw args as in JuliaLang/julia#22646 for 0.6.

cc @TsurHerman

@Keno
Copy link
Collaborator

Keno commented Jul 25, 2017

So @vtjnash should correct me here, but I think this is the wrong place to do it. Instead, pyjulia should update the worldcounter in the ptls whenever it has imported some code. I'm just not sure if there's currently a C Api for that.

src/callback.jl Outdated
@@ -8,7 +8,7 @@
################################################################

# Define a Python method/function object from f(PyPtr,PyPtr)::PyPtr.
# Requires f to be a top-level function.
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo

@Keno
Copy link
Collaborator

Keno commented Jul 25, 2017

On the other hand, I also don't really see a downside to doing this here.

src/callback.jl Outdated
@@ -32,11 +32,16 @@ function jl_Function_call(self_::PyPtr, args_::PyPtr, kw_::PyPtr)
try
f = unsafe_pyjlwrap_to_objref(self_)::Function
if kw_ == C_NULL
ret = PyObject(f(convert(PyAny, args)...))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This package still works on 0.5, so you need a check here e.g. isdefined(Base, :invokelatest).

@vtjnash
Copy link
Contributor

vtjnash commented Jul 25, 2017

jl_Function_call is already wrapped in a trampoline (via cfunction) which does the equivalent of invokelatest, so this shouldn't be necessarily (unless JuliaLang/julia#20167 isn't doing its job correctly)

remove debugging printout
@ihnorton
Copy link
Member Author

ihnorton commented Jul 26, 2017

Updated.

@vtjnash I don't know whether that PR is doing its job, but there's a simple repro of the issue this solves, here: JuliaPy/pyjulia#105 (comment)

@codecov-io
Copy link

Codecov Report

Merging #416 into master will increase coverage by 0.17%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #416      +/-   ##
==========================================
+ Coverage   66.04%   66.22%   +0.17%     
==========================================
  Files          17       17              
  Lines        1511     1516       +5     
==========================================
+ Hits          998     1004       +6     
+ Misses        513      512       -1
Impacted Files Coverage Δ
src/callback.jl 95.23% <100%> (+3.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bde073b...df1d16c. Read the comment docs.

@Keno
Copy link
Collaborator

Keno commented Jul 28, 2017

Ok, so the underlying problem here will be addressed by JuliaLang/julia#22987. However, we should probably still merge this here to fix this on current versions of 0.6. Once base is merged, we can put in an appropriate version check.

@Keno Keno merged commit 70057b4 into JuliaPy:master Jul 28, 2017
@stevengj
Copy link
Member

Thanks!

@ihnorton ihnorton deleted the fix343_invokelatest branch September 18, 2017 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MethodError when using (indirectly) StaticArrays failure on 0.6 in anonymous callback
5 participants