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

Fix most of 0.7 depwarns #425

Merged
merged 2 commits into from
Sep 30, 2017
Merged

Fix most of 0.7 depwarns #425

merged 2 commits into from
Sep 30, 2017

Conversation

yuyichao
Copy link
Collaborator

No description provided.

@yuyichao
Copy link
Collaborator Author

Updated to include more fixes. One depwarn waiting for JuliaLang/Compat.jl#400 and not included.

@yuyichao
Copy link
Collaborator Author

Note that getting loading to pass on nightly requires FluxML/MacroTools.jl#57

@yuyichao
Copy link
Collaborator Author

Ping.

1 similar comment
@yuyichao
Copy link
Collaborator Author

Ping.

@yuyichao
Copy link
Collaborator Author

Ping. Added some more fix now that the Compat change is merged. CI pass will still need the MacroTools PR.

@yuyichao
Copy link
Collaborator Author

Ping, anyone? @stevengj ?

This PR has been sitting here for a month now. And have been updated 3 times to include new fixes...............................

@yuyichao
Copy link
Collaborator Author

And ref JuliaPy/Conda.jl#82 (comment) JuliaPy/Conda.jl#67 (comment) about organization permission setup.....

src/pyinit.jl Outdated
argv = unsafe_convert(Ptr{Cwchar_t}, argv_s)
ccall(@pysym(:PySys_SetArgvEx), Void, (Cint, Ptr{Ptr{Cwchar_t}}, Cint), 1, &argv, 0)
end
ccall(@pysym(:PySys_SetArgvEx), Void, (Cint, Ptr{UInt32}, Cint), 1, EmptyStringList(), 0)
Copy link
Member

Choose a reason for hiding this comment

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

Isn't there a simpler way to do this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Keeping the branch and using Ptr{Cstring}, Ptr{Cwstring} and pass in an actual array of string should work too. I did it this way since,

  1. This eliminate the branch.
  2. So the code is actually not much longer
  3. I want to see how easy it is to implement sth like this properly
  4. It doesn't allocate anything (optimized out on 0.7) (yeah, I know it's just __init__)

When 0.6 support is dropped, this can be done with @gc_preserve without external definition.

ref0 = Ref{UInt32}(0)
@gc_preserve ref0 ccall(@pysym(:PySys_SetArgvEx), Void, (Cint, Ref{Ptr{Void}}, Cint), 1, pointer_from_objref(ref0), 0)

Copy link
Member

Choose a reason for hiding this comment

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

The branch and the performance are irrelevant here. But as your code shows, we don't actually need the branch, since a pointer to a pointer to an array of a 32-bit zero will work in both Python 2 and 3 (even though Python 2 needs only one byte of zero).

I would prefer to just use Ptr{Cwstring} to pass an an actual array rather than declaring a custom type here.

@StefanKarpinski
Copy link

@yuyichao, I changed the default member privilege to write so you should be able to merge this yourself now – let me know if you can't.

@yuyichao
Copy link
Collaborator Author

I can merge this now. I do want to wait for a new nightly though since the code here exposes a base bug....

Makes sure that the pointer lifetime is managed by a mutable object.
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.

3 participants