Re: [WideChar conversion improvements - 2/5] Implement new versions ofwctomb and wcstombs

[Dmitry Timoshkov]

"Colin Finck" <mail(a)colinfinck.de> wrote:
> -INT CDECL MSVCRT_wctomb( char *dst, MSVCRT_wchar_t ch )
> +INT CDECL MSVCRT_wctomb(char *mbchar, MSVCRT_wchar_t wchar)

What's the point of the above change?
> {
> -  return WideCharToMultiByte( CP_ACP, 0, &ch, 1, dst, 6, NULL, NULL );
> +    BOOL bUsedDefaultChar;
> +    char chMultiByte[MSVCRT_MB_LEN_MAX];
> +    int nBytes;

Please don't use ugly mixed case variable names. Same for other tests.
> +
> +    /* At least one parameter needs to be given, the length of a null character cannot be queried (verified by tests 
> under WinXP SP2) */
> +    if(!mbchar && !wchar)
> +        return 0;

Please keep line length close to 80 columns limit. What are you calling a
"null character" here? A NULL pointer is not a NUL character. What happens
in the code below if mbchar is not NULL but wchar is?
> +
> +    /* Use WideCharToMultiByte for doing the conversion using the codepage currently set with setlocale() */
> +    nBytes = WideCharToMultiByte(MSVCRT___lc_codepage, 0, &wchar, 1, chMultiByte, MSVCRT_MB_LEN_MAX, NULL, 
> &bUsedDefaultChar);

-- 
Dmitry.

[WideChar conversion improvements - 2/5] Implement new versionsofwctomb and wcstombs

[Colin Finck]

Dmitry Timoshkov wrote:
> > -INT CDECL MSVCRT_wctomb( char *dst, MSVCRT_wchar_t ch )
> > +INT CDECL MSVCRT_wctomb(char *mbchar, MSVCRT_wchar_t wchar)
>
> What's the point of the above change?

While rewriting that function, I used the parameter names described on MSDN.
I don't see a point in inventing new parameter names for documented 
functions.
> Please don't use ugly mixed case variable names. Same for other tests.

Well, this is the variable naming convention used by most Win32 developers.
Also there are a lot of other components in Wine using this coding style, 
just look for example in "notepad" or "regedit".
> What are you calling a "null character" here? A NULL pointer is not a NUL 
> character. What happens
> in the code below if mbchar is not NULL but wchar is?

if(mbchar && !wchar)  --> Put a NULL character into mbchar and return 1 as 
its length.
if(!mbchar && wchar)  --> Return the length of wchar

You might think that (!mbchar && !wchar) should now return 1 as the length 
of a NULL character, but MS seems to have put a check here and returns 0 in 
this case.

These behaviours are also covered by some of the added tests in the 4th 
patch.

Best regards,

Colin

[WideChar conversion improvements - 2/5] Implement new versionsofwctomb and wcstombs

[Colin Finck]

Dmitry Timoshkov wrote:
> > -INT CDECL MSVCRT_wctomb( char *dst, MSVCRT_wchar_t ch )
> > +INT CDECL MSVCRT_wctomb(char *mbchar, MSVCRT_wchar_t wchar)
>
> What's the point of the above change?

While rewriting that function, I used the parameter names described on MSDN.
I don't see a point in inventing new parameter names for documented
functions.
> Please don't use ugly mixed case variable names. Same for other tests.

Well, this is the variable naming convention used by most Win32 developers.
Also there are a lot of other components in Wine using this coding style,
just look for example in "notepad" or "regedit".
> What are you calling a "null character" here? A NULL pointer is not a NUL
> character. What happens
> in the code below if mbchar is not NULL but wchar is?

if(mbchar && !wchar)  --> Put a NULL character into mbchar and return 1 as
its length.
if(!mbchar && wchar)  --> Return the length of wchar

You might think that (!mbchar && !wchar) should now return 1 as the length
of a NULL character, but MS seems to have put a check here and returns 0 in
this case.

These behaviours are also covered by some of the added tests in the 4th
patch.

Best regards,

Colin

[WideChar conversion improvements - 2/5] Implement new versionsofwctomb and wcstombs

[Vitaliy Margolen]

Colin Finck wrote:
> Dmitry Timoshkov wrote:
> > > -INT CDECL MSVCRT_wctomb( char *dst, MSVCRT_wchar_t ch )
> > > +INT CDECL MSVCRT_wctomb(char *mbchar, MSVCRT_wchar_t wchar)
> > > What's the point of the above change?
>
> While rewriting that function, I used the parameter names described on MSDN.
> I don't see a point in inventing new parameter names for documented
> functions.
> That doesn't mean they have to be used in Wine. Please don't do that.
> > Please don't use ugly mixed case variable names. Same for other tests.
>
> Well, this is the variable naming convention used by most Win32 developers.
> That doesn't make it right in the *NIX world. Please use notation more 
> accepted in Wine - all lover case with underscores.
> > What are you calling a "null character" here? A NULL pointer is not a NUL
> > character. What happens
> > in the code below if mbchar is not NULL but wchar is?
>
> if(mbchar && !wchar)  --> Put a NULL character into mbchar and return 1 as
> its length.
> NULL is a null pointer. NUL is a \0 character. Please don't mix and match 
> the two. They are totally separate things.
> These behaviours are also covered by some of the added tests in the 4th
> patch.
> Then those particular tests should go together with this patch to show it's 
> correct.

Vitaliy.