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 for cwstring, cconvert, transcode when !is_windows() #17410

Closed

Conversation

iagobaapellaniz
Copy link
Contributor

On line 78 the function cconvert() calls for transcode(Cwchar_t, String(s).data) but latter it is not method for trancode(::Type{UInt32}, Array{UInt8, 1}) on non windows machines.

I also chaged Cwchar_t from Int32 to UInt32, since I think it could be a typo.

This allows to pass using PyCall definitelly. See issue, PyCall/#295.

More tests, with PyCall to come.

@iagobaapellaniz iagobaapellaniz changed the title Fix for cwstring, convert, transcode when !is_windows() Fix for cwstring, cconvert, transcode when !is_windows() Jul 14, 2016
typealias Cwchar_t Int32
typealias Clong Int64
typealias Culong UInt64
typealias Cwchar_t UInt32
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed from aliases to normal names, so it is clearer what's the difference between Windows and the rest of systems.

@iagobaapellaniz
Copy link
Contributor Author

Should Cwchar_t be UInt32 or Int32 ??. I think it should be UInt32, as it is for Windows systems and as it is required by the methods called with this argument, for instance transcode()

@@ -23,7 +23,7 @@ if is_windows()
else
typealias Clong Int
typealias Culong UInt
typealias Cwchar_t Int32
typealias Cwchar_t UInt32
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change? wchar_t seems to be signed here.

Copy link
Contributor Author

@iagobaapellaniz iagobaapellaniz Jul 14, 2016

Choose a reason for hiding this comment

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

I know that Cwchar_t is passed as argument to the function transcode and it seems to accept on all the cases unsigned integers. So I changed the alias from Int32 to UInt32. Also now it is coherent with the definition of the aliases for windows, few lines above.

On the definition of transcode{T::Union{UInt8, UInt16}(::Type{T}, src::T)=src, line 143 and beyond.

I'm trying to build with appveyor but it is still running. I was able to build it locally with no error

Copy link
Contributor

Choose a reason for hiding this comment

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

Consistency with windows is not the point for this definition. This is what the C type is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, changing this definition will be subtly breaking. Functions that need to use Cwchar_t should just be fixed to handle both cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not expertise on it, should I reverse it to Int32? Aren't the used to represent characters and therefore negatives are not useful?

Maybe Int32 is faster than UInt32 even if UInt32 doesn't break anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

should I reverse it to Int32

Yes.

Aren't the used to represent characters and therefore negatives are not useful?

Blame whoever define them to be signed in C. Cchar is also signed on linux afaik.

@iagobaapellaniz
Copy link
Contributor Author

iagobaapellaniz commented Jul 14, 2016

You all did the work instead me! Thank you. At least this fix let me use PyCall, and plot figures with PyPlot.

Squashed all commits into one.

Fix for cwstring, convert, transcode when !is_windows()

Fix thanks for 32-bits unix

Fix is reusing code now for transcode()

Dst_et by T

Updated transcode method
@stevengj
Copy link
Member

stevengj commented Jul 14, 2016

The PyCall problem is already fixed in PyCall master.

@stevengj
Copy link
Member

transcode supports both Int32 and UInt32, so defining Cwchar_t as Int32 is not a problem for transcode. It would seem cleaner if we defined it as UInt32, I agree. The C standard does not specify whether wchar_t is signed.

On MacOS and Linux gcc, however, it looks like wchar_t is a (signed) int by default, so I think we were copying that. (In principle, it probably wouldn't hurt if we used UInt32 even if the C type is Int32, but for the most part we try to match signed-ness with the Cxxxx constants.)

@tkelman
Copy link
Contributor

tkelman commented Jul 14, 2016

I would defer to / prefer being consistent with what GCC does on Linux, and what Clang does on Mac (if different than GCC).

@iagobaapellaniz
Copy link
Contributor Author

I think that to be consistent, I should have to work on the reverse transcode

I've updated the code adding the transcode(::Type{Int32}, src::Array{UInt8,1}). I'll work on the reverse function for completeness.

@tkelman tkelman added the needs tests Unit tests are required for this change label Jul 15, 2016
@stevengj
Copy link
Member

@i-apellaniz, I think you may be looking at an out-of-date version of Julia. Int32 support (in both directions) was already added to transcode in #17323.

@iagobaapellaniz
Copy link
Contributor Author

Yes! It is added already. Let me testt it againts PyCall, and soon if everything is ok, i'll close this pull request.

Thanks all!

@iagobaapellaniz iagobaapellaniz deleted the fixForCwstr branch July 15, 2016 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Unit tests are required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants