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 depwarn on 0.5 #267

Merged
merged 1 commit into from May 14, 2016
Merged

Fix depwarn on 0.5 #267

merged 1 commit into from May 14, 2016

Conversation

yuyichao
Copy link
Collaborator

@yuyichao yuyichao commented May 7, 2016

String, Symbol, precision(BigFloat)
Also fix an incorrect use of unsafe_convert.

Requires a new tag of Compat.
Also includes #260

Requires JuliaLang/Compat.jl#192

@@ -1,4 +1,4 @@
julia 0.4
Compat 0.7.14
Compat 0.7.14+
Copy link
Member

Choose a reason for hiding this comment

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

Presumably we should wait until the necessary version of Compat is tagged?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct. I'm just not sure what's the status of JuliaLang/Compat.jl#192 .

@yuyichao
Copy link
Collaborator Author

yuyichao commented May 8, 2016

General comment about passing julia object to C.
If you want to pass an julia object v to C in the same format it would have been passed in ccall as an argument of type T with explicit control of the object lifetime, the correct way to do this is (with the exception if T is Any since it is handled specially),

root = Base.cconvert(T, v) # this object controls the lifetime
cobj = Base.unsafe_convert(T, v) # this is the C value

The name of the pointer field comes from pointerfree. I can rename it to GC root if it's more clear.

@stevengj
Copy link
Member

stevengj commented May 8, 2016

Oh, right. The cconvert function is very confusingly named, because I keep expecting it to act like convert; it's weird that cconvert(T, v) does not produce an object of type T. Maybe gcroot would be a more descriptive name.

@yuyichao
Copy link
Collaborator Author

yuyichao commented May 8, 2016

Maybe gcroot would be a more descriptive name.

gc_convert ;-p (not sure if it actually worth changing the name though....)

Another note is that pointer should generally be avoided whenever possible. It is usually only necessary for interacting with C code (ok, I admit I've used it to workaround LLVM vectorization issue......) and in such case the cconvert, unsafe_convert is much safer (despite possibly confusing names.....) and should automatically happen for ccall.

@yuyichao
Copy link
Collaborator Author

yuyichao commented May 8, 2016

It's weird that cconvert(T, v) does not produce an object of type T.

It's true that it can return whatever type it feels like as long as it keeps the result of unsafe_convert on the same type alive. This indeed doesn't sound like what a *convert should do (except that it is part of the full conversion (cconvert + unsafe_convert) to a C object) so I now see why it is confusing.......... (started in 0.4 time, I've never seen the old interface and have naturally accepted the new one....)

@yuyichao
Copy link
Collaborator Author

Updated.

There is one API change here. I take this as it doesn't matter....

@coveralls
Copy link

coveralls commented May 11, 2016

Coverage Status

Coverage decreased (-0.5%) to 58.895% when pulling baa6180 on yuyichao:0.5-deps into eb8d70a on stevengj:master.

@stevengj
Copy link
Member

Yeah, using ByteString there on 0.5 should be fine.

@stevengj
Copy link
Member

Travis failure:

ERROR: LoadError: PyError (:PyObject_Call) <type 'exceptions.TypeError'>
TypeError('type() argument 1 must be string, not unicode',)

Probably the string conversion needs to be:

function PyObject(s::AbstractString)
    sb = String(s)
    if pyunicode_literals || !isascii(sb)
        PyObject(@pycheckn ccall(@pysym(PyUnicode_DecodeUTF8),
                                 PyPtr, (Ptr{UInt8}, Int, Ptr{UInt8}),
                                 sb, sizeof(sb), C_NULL))
    else
        PyObject(@pycheckn ccall(@pysym(PyString_FromStringAndSize),
                                 PyPtr, (Ptr{UInt8}, Int), sb, sizeof(sb)))
    end
end

and you can get rid of PyObject(::String).

@yuyichao
Copy link
Collaborator Author

............ python unicode coupled with julia string change ...........

Yeah, I only tested on python3.... I'll fix it soon.

String, Symbol, precision(BigFloat)
Also fix an incorrect use of unsafe_convert
@yuyichao
Copy link
Collaborator Author

Done

@coveralls
Copy link

coveralls commented May 11, 2016

Coverage Status

Coverage decreased (-0.07%) to 59.354% when pulling 6ff7415 on yuyichao:0.5-deps into eb8d70a on stevengj:master.

@yuyichao
Copy link
Collaborator Author

ping.

@stevengj stevengj merged commit 97ba3f0 into JuliaPy:master May 14, 2016
@stevengj
Copy link
Member

Thanks!

@yuyichao yuyichao deleted the 0.5-deps branch May 14, 2016 01:12
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.

None yet

3 participants