[MPlayer-dev-eng] [PATCH]can not open ISO file including multibyte file path. on Windows.

[Reimar Döffinger]

On Mon, Apr 24, 2017 at 03:27:59AM +0900, Tadashi Ando wrote:
> Can you OK this patch ?

It looks OK to me, I'll try to apply it during the weekend.
I have a few minor style comments, I'd fix them myself
when applying but I'll mention them here in case you
have objections/want to change it yourself/...

> +wchar_t* utf8_to_wide_char( const char* utf8 )

MPlayer style (as far as it exists) is to have
the space before *, not after (as that is
in some cases less misleading).


> +    

If the script is still working, "empty"
lines that contain spaces can't be committed.

> +    wchar_t* wide_char = NULL;
> +    
> +    wide_char_size = MultiByteToWideChar(CP_UTF8, MB_ERR_INVALID_CHARS, utf8, -1, wide_char, 0);

These can be merged. There are a few more of these.

> +    wide_char = calloc(wide_char_size, sizeof(wchar_t));

We generally prefer and consider it safer to
write sizeof(*wide_char) instead.

> +    ansi = calloc(1, ansi_size);

Not that it matters, but the argument order here seems a bit
inconsistent with the calloc before.

> +#if defined(__MINGW32__) || defined(__CYGWIN__)
> +wchar_t* utf8_to_wide_char(const char* utf8);
> +char* wide_char_to_local_windows_code_page(const wchar_t* wide_char);
> +char* utf8_to_local_windows_code_page(const char* utf8);
> +#endif

Except for the wchar_t possibly causing issues
the #if probably isn't necessary.
But might be safer to have it.

> +        char* ansi_dvd_device_current = NULL;
> +        
> +        if ( ansi_dvd_device_current = utf8_to_local_windows_code_page(dvd_device_current) )

I really strongly dislike assignments in ifs.
A selection of reasons:
1) like this, it will cause warnings in many compilers
2) the reason for that being that it might have been
a typo of writing = when == was meant and everyone
reading it will have to be careful that they read it correctly.
3) it makes for worse readability (because of above reasons, but
also since it discourages adding line breaks even when appropriate
as that looks really bad inside an if, because conditions
are always complex to understand and one shouldn't add more
complexity in the middle of them etc.)
4) in this case it doesn't even reduce the number of lines
of code, as it count have been assigned when the variable was declared.

Regards,
Reimar