Improve compatibility between Cstring and Ptr. #14024

maleadt · 2015-11-17T10:45:03Z

This addresses #13974, and adds some convenience methods (pointer, and promotion rules) for working with Cstrings.

I've already defined proper promotion rules, but haven't found out how to get a commutative equality operator without defining two versions. If somebody could give some hints about that, I'll still fix that.

StefanKarpinski · 2015-11-17T15:52:45Z

The promotion fallback for == only applies to Numbers. Can you do this with just the two == methods without the promotion rule? For some reason this promotion strikes me as somewhat sketchy.

stevengj · 2015-11-17T15:56:17Z

Yes, the promotion rule seems unnecessary here. Just define ==(x::Union{Cstring,Cwstring}, y::Ptr) = pointer(x) == y and vice versa. (Note that {T} is unnecessary here.)

stevengj · 2015-11-17T15:59:33Z

Might also be nice to have a Cstring(p::Ptr{Void}) constructor so that you can do Cstring(C_NULL) rather than Cstring(Ptr{UInt8}(C_NULL)). It could throw an exception for p != C_NULL.

maleadt · 2015-11-17T18:55:23Z

Okay, will address both tomorrow.

If this is merged, does it make sense to convert usages of Ptr{UInt8} to Cstring in base (where semantically applicable, of course), or is this too much churn for what it's worth?

stevengj · 2015-11-17T20:42:16Z

@maleadt, the important thing is to convert ccall arguments where NUL-terminated strings are expected, and I think I already did this everywhere a check is needed (unless more have been added in the meantime). I don't think it is worthwhile converting other usages of Ptr{UInt8}.

nalimilan · 2015-11-17T21:00:52Z

...but updating the manual is certainly a good idea.

maleadt · 2015-11-18T12:42:24Z

@stevengj I've actually spotted quite some -- 26, to be exact -- places where Ptr{UInt8} strings (ie. should not contain NULL bytes) could be converted to Cstring. I'll push it here soon, I first want to see what the Windows bot thinks about my last commit.

@nalimilan What would you like to see added? The Cstring usage is pretty well-documented already, and I haven't changed any function signature.

nalimilan · 2015-11-18T13:25:17Z

@nalimilan What would you like to see added? The Cstring usage is pretty well-documented already, and I haven't changed any function signature.

In #13974, you mentioned that this would allow using Cstring as a return type in addition to an argument type, so I thought the manual could be adapted to recommend this instead of Ptr{UInt8}. But there are still uses of Ptr{UInt8} as an argument type, which could have been converted to Cstring too, so that's not a new issue.

maleadt · 2015-11-25T08:12:43Z

Rebase and force push in order to kickstart appveyor (I don't think the previous failure was caused by me). I'll update the docs too.

stevengj · 2015-11-25T14:09:02Z

base/fft/FFTW.jl

@@ -101,16 +101,16 @@ alignment_of(A::FakeArray) = Int32(0)
 # FFTW's api/import-wisdom-from-file.c file].

 function export_wisdom(fname::AbstractString)
-    f = ccall(:fopen, Ptr{Void}, (Cstring,Ptr{UInt8}), fname, "w")
+    f = ccall(:fopen, Ptr{Void}, (Cstring,Cstring), fname, "w")


Note that Ptr{UInt8} is safe for things like this because we know that the literal string "w" contains no embedded NUL chars. So in my initial Cstring PR I left these as-is as an optimization (albeit probably pointless).

It looks like just about all of the Ptr{UInt8}-to-Cstring substitutions that you made in argument lists are of this form.

Right, though if you have the time/will to add a short comment explaining this, it wouldn't hurt, and will avoid repeating this next time somebody bumps on it.

Note that Cstring generates much longer code.

A cleaner solution: change these arguments to symbols. convert already skips null checking for them since they cannot contain NULLs.

OK, will be changing to symbols where appropriate, and back to Ptr{UInt8} otherwise. The IR generated by the Cstring conversions is remarkably long indeed, but performance seemed to be okay. Will have a closer look.

Main use case: use Cstring as return value of ccall's which potentially return C_NULL.

maleadt · 2015-12-01T07:12:04Z

Rebased on top of master, and squashed the commits into three logical pieces (core changes, documentation, and adaptation of base).

tkelman · 2015-12-01T07:19:43Z

doc/manual/calling-c-and-fortran-code.rst


-    julia> (Ptr{UInt8})


guess these could be doctests?

OK. Do I need to do anything after converting the block to .. doctest::? The file doesn't seem to get picked up by make -C doc doctest

oh strange, maybe it's not a big enough deal to be worth worrying about then

My mistake, I didn't realize :: started a literal block. Will update the documentation commit in a sec.

yuyichao · 2015-12-01T13:03:13Z

base/c.jl

 convert{T<:Union{Int8,UInt8}}(::Type{Cstring}, p::Ptr{T}) = box(Cstring, p)
 convert(::Type{Cwstring}, p::Ptr{Cwchar_t}) = box(Cwstring, p)
 convert{T<:Union{Int8,UInt8}}(::Type{Ptr{T}}, p::Cstring) = box(Ptr{T}, p)
 convert(::Type{Ptr{Cwchar_t}}, p::Cwstring) = box(Ptr{Cwchar_t}, p)

+# construction from untyped pointers
+convert{T<:Union{Cstring,Cwstring}}(::Type{T}, p::Ptr{Void}) =
+    p==C_NULL ? box(Cstring, p) : throw(ArgumentError("cannot convert non-null void pointer to C string"))


Is ther any problem with converting non-NULL Ptr{Void} to Cstring? Also, isn't this always returning Cstring even when called with Cwstring?

Then how would bytestring(Cstring) behave? Currently, it only checks for ptr != C_NULL, relying on the fact that ptr points to a NULL-terminated array of chars. I'm not sure what the advantage would be of allowing Cstring to contain an arbitrary Ptr{Void}

And yes, it only constructs Cstrings... Any suggestion? The constructor was based on @stevengj's comment.

Then how would bytestring(Cstring) behave? Currently, it only checks for ptr != C_NULL, relying on the fact that ptr points to a NULL-terminated array of chars.

I don't see why does this matter. In another word, I don't think it is necessary to make this convert method only defined for invalid pointers. reinterpret already works and if you are adding another method to do the conversion you might as well make them the same. I don't think you will gain any safety by not allowing a non-NULL pointer to be converted to Cstring anyway.

I'm not sure what the advantage would be of allowing Cstring to contain an arbitrary Ptr{Void}

Much easier to inline, less code to write, less code to generate, in the cases the current version is inlined, less work for compiler to remove useless code.

And yes, it only constructs Cstrings... Any suggestion? The constructor was based on @stevengj's comment.

Just replace box(Cstring, p) with box(T, p) as what you would usually do with a parametrized method.

Sure, fine by me. I've pushed a commit removing the check and related test.

maleadt · 2015-12-01T15:52:02Z

I don't think the Travis error is caused by me, but I'm not sure. Is there any way to request a rebuild?

tkelman · 2015-12-01T18:05:50Z

looks like someone restarted it. what was the error?

maleadt · 2015-12-01T18:08:10Z

Something in test/spawn on the Travis bot running Mac.

yuyichao · 2015-12-01T18:16:43Z

I restarted it. The error was a Signal 2 and a Segfault during the test/spawn test in the serializer (dump.c).

tkelman · 2015-12-01T18:17:34Z

that sounds new. i would've saved and gisted the log for that...

stevengj · 2015-12-01T20:46:48Z

LGTM.

Improve compatibility between Cstring and Ptr.

maleadt · 2015-12-08T06:18:34Z

Thanks for merging.

stevengj reviewed Nov 25, 2015
View reviewed changes

maleadt mentioned this pull request Nov 25, 2015

Builtins returning signed integers #14137

Closed

Improve compatibility between Cstring and Ptr.

7e28d30

Main use case: use Cstring as return value of ccall's which potentially return C_NULL.

tkelman reviewed Dec 1, 2015
View reviewed changes

maleadt added 2 commits December 1, 2015 09:05

Use Cstring in the documentation, and explain its use better.

04d57fe

Replace some Ptr{UInt8} string usages to Cstring.

66f34ba

yuyichao reviewed Dec 1, 2015
View reviewed changes

Allow all conversions from Ptr{Void} to Cstring.

40f2d85

yuyichao mentioned this pull request Dec 2, 2015

Aways make a copy of the finalizer list to run before running finalizers #14181

Merged

jakebolewski added a commit that referenced this pull request Dec 7, 2015

Merge pull request #14024 from maleadt/pr_cstring

52c1c67

Improve compatibility between Cstring and Ptr.

jakebolewski merged commit 52c1c67 into JuliaLang:master Dec 7, 2015

maleadt mentioned this pull request Dec 8, 2015

Cstring(NULL) does not equal C_NULL #13974

Closed

maleadt deleted the pr_cstring branch April 20, 2016 12:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve compatibility between Cstring and Ptr. #14024

Improve compatibility between Cstring and Ptr. #14024

maleadt commented Nov 17, 2015

StefanKarpinski commented Nov 17, 2015

stevengj commented Nov 17, 2015

stevengj commented Nov 17, 2015

maleadt commented Nov 17, 2015

stevengj commented Nov 17, 2015

nalimilan commented Nov 17, 2015

maleadt commented Nov 18, 2015

nalimilan commented Nov 18, 2015

maleadt commented Nov 25, 2015

stevengj Nov 25, 2015

nalimilan Nov 25, 2015

yuyichao Nov 25, 2015

nalimilan Nov 25, 2015

maleadt Nov 25, 2015

maleadt commented Dec 1, 2015

tkelman Dec 1, 2015

maleadt Dec 1, 2015

tkelman Dec 1, 2015

maleadt Dec 1, 2015

yuyichao Dec 1, 2015

maleadt Dec 1, 2015

yuyichao Dec 1, 2015

maleadt Dec 1, 2015

maleadt commented Dec 1, 2015

tkelman commented Dec 1, 2015

maleadt commented Dec 1, 2015

yuyichao commented Dec 1, 2015

tkelman commented Dec 1, 2015

stevengj commented Dec 1, 2015

maleadt commented Dec 8, 2015

Improve compatibility between Cstring and Ptr. #14024

Improve compatibility between Cstring and Ptr. #14024

Conversation

maleadt commented Nov 17, 2015

StefanKarpinski commented Nov 17, 2015

stevengj commented Nov 17, 2015

stevengj commented Nov 17, 2015

maleadt commented Nov 17, 2015

stevengj commented Nov 17, 2015

nalimilan commented Nov 17, 2015

maleadt commented Nov 18, 2015

nalimilan commented Nov 18, 2015

maleadt commented Nov 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maleadt commented Dec 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maleadt commented Dec 1, 2015

tkelman commented Dec 1, 2015

maleadt commented Dec 1, 2015

yuyichao commented Dec 1, 2015

tkelman commented Dec 1, 2015

stevengj commented Dec 1, 2015

maleadt commented Dec 8, 2015