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

[win32] CharsetConverter: Fix: allow to use correctly UTF-32 on windows #3342

Closed
wants to merge 1 commit into from

Conversation

Karlson2k
Copy link
Member

Seems to be a bug (wrong compiler configuration) in our supplied libiconv

This PR will not break anything as the only "old" utf32ToStringCharset function was unused.

Seems to be a bug (wrong compiler configuration) in our supplied libiconv
@Karlson2k
Copy link
Member Author

jenkins build this please

@jmarshallnz
Copy link
Contributor

What are you trying to fix here? i.e. what is UTF-32 giving you on win32 that you don't think is right?

@Karlson2k
Copy link
Member Author

Conversion to UTF-32 with current lib give the same result as UTF-32BE. This is wrong as it should be UTF-32LE.
I found it when trying to process result with FriBiDi (that works internally only with UTF-32)

Think this is a main reason why all those defs are here - fix wrong endianness on Win32. Fortunately UTF-8 is endianness agnostic so we don't have much troubles until now (except "UTF-16").

@jmarshallnz
Copy link
Contributor

That seems most strange to me. Can you see from the iconv sources why this might be?

@Karlson2k
Copy link
Member Author

I'll check it.
Was trying to build iconv lib for debug, but instruction in libiconv-1.13.1-win32.zip/readme.txt is incorrect. Need to prepare correct headers from .h.in

@Karlson2k
Copy link
Member Author

As soon as I replace UTF-16LE with UTF-16 in define everything goes wrong.
Going to check deeper.

@Karlson2k
Copy link
Member Author

Surprise! This is intentional. At least for GNU's libiconv
Comments from code:

/* We output UTF-16 in big-endian order, with byte-order mark.
   See RFC 2781 section 3.3 for a rationale: Some document formats
   mandate a BOM; the file concatenation issue is not so severe as
   long as the above utf16_mbtowc function is used. */

and

/* We output UTF-32 in big-endian order, with byte-order mark. */

'BE' and 'LE' versions don't output BOM

So this fix is valid.

But I need to check other iconv implementation. Looks like for wide string it's better to use WCHAR_T as it doesn't have UTF-32 limitations and doesn't have surrogate pairs on Win32. If other implementations is similar, I'll publish another PR with WCHAR_T fix.

@wsoltys
Copy link

wsoltys commented Sep 28, 2013

I'll check it.
Was trying to build iconv lib for debug, but instruction in libiconv-1.13.1-win32.zip/readme.txt is incorrect. Need to prepare correct headers from .h.in

Whats wrong with it? Please elaborate and correct it.
Note: if you wanna update libiconv please don't forget to update the mingw iconv files as well (and all stuff which depends on it). I'm not quite sure anymore if this is really needed but afaik it shares at least the runtime dll with the vs stuff.

@Karlson2k
Copy link
Member Author

@wsoltys Missing steps in readme.txt

  1. Run ./configure in MinGW
  2. Put libiconv-xx to xbmc's libs/win32
  3. Delete mingw generated config.h
    Then follow readme.txt instructions.
    I'll correct it.

@wsoltys
Copy link

wsoltys commented Sep 28, 2013

I won't say that I did it right but why do you want to run mingw configure and compile it afterwards with vs? Does it provide another iconv header or for what is it good?
Next question, why put it into lib/win32? Ready compiled packages go into project\BuildDependencies\lib and as long we don't do much changes to the vanilla source they shouldn't be in our repo.

@Karlson2k
Copy link
Member Author

@wsoltys "configure" generates real '.h' files from '.h.in', vs2010 project unable to do so. Generated headers can be put into package, to allow simple compilation with vs project.

Need to put it to 'lib/win32' (or any other folder in XBMC tree in form 'foo1/bar2') for compilation as libiconv_win32.vcxproj has

@wsoltys
Copy link

wsoltys commented Sep 30, 2013

That might be a leftover from when it was in tree. Fix it, compile it off tree and make it a dep. I don't think that it depends on anything in our tree so the props might not be needed (but could be wrong as it's some time ago ;)

Am 30.09.2013 um 01:26 schrieb Karlson2k notifications@github.com:

@wsoltys "configure" generates real '.h' files from '.h.in', vs2010 project unable to do so. Generated headers can be put into package, to allow simple compilation with vs project.

Need to put it to 'lib/win32' (or any other folder in XBMC tree in form 'foo1/bar2') for compilation as libiconv_win32.vcxproj has


Reply to this email directly or view it on GitHub.

@Karlson2k
Copy link
Member Author

Better solution: #3353

@Karlson2k Karlson2k closed this Oct 2, 2013
@Karlson2k Karlson2k deleted the charsetconverter_fix_02 branch October 15, 2013 11: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.

3 participants