Re: kernel32: Add UTF-7 support

[Alex Henrie]

2012/5/7 Ben Klein :
> Without being a regular contributor, here are a few tips from a cursory glance:
> - Whitespace change on at least one line

The only change to whitespace I made was:

-        if( flags)
+        if (flags)

I don't think this is a big deal.
> - Why the change from ERROR_INVALID_PARAMETER to ERROR_INVALID_FLAGS?

To match the behavior of Windows. I verified this with both MSDN and
empirical tests. If the flags parameter is not null, all versions of
Windows tested report ERROR_INVALID_FLAGS with the possible exception
of Windows NT4.
See http://testbot.winehq.org/JobDetails.pl?Key=18148
> - New function defs do not match format of surrounding defs in unicode.h

This was a hard one. I didn't know if I should keep Katayama's use of
INT or change the functions to use int like the UTF-8 functions do. So
you're saying it should all be int? Do LPCSTR and LPCWSTR need to be
changed to something else as well?
> - No new tests to confirm the behaviour is correct (or mention of
>
> current tests succeeding)

dlls/kernel32/tests/locale.c already has just as many UTF-7 tests as
it has UTF-8 tests. What tests do we need for UTF-7 which are not
needed for UTF-8?

Thanks for your quick response,

-Alex

kernel32: Add UTF-7 support

[Ben Klein]

On 8 May 2012 12:50, Alex Henrie  wrote:
> 2012/5/7 Ben Klein :
> > Without being a regular contributor, here are a few tips from a cursory glance:
> > - Whitespace change on at least one line
>
> The only change to whitespace I made was:
> - if( flags)
> - if (flags)
>
> I don't think this is a big deal.

Sure, but you might as well fix it for the next submission.
> > - Why the change from ERROR_INVALID_PARAMETER to ERROR_INVALID_FLAGS?
>
> To match the behavior of Windows. I verified this with both MSDN and
> empirical tests. If the flags parameter is not null, all versions of
> Windows tested report ERROR_INVALID_FLAGS with the possible exception
> of Windows NT4.
> See http://testbot.winehq.org/JobDetails.pl?Key=18148

I see. In that case, this should go in a separate patch as it's not
specific to UTF7 support.
> > - New function defs do not match format of surrounding defs in unicode.h
>
> This was a hard one. I didn't know if I should keep Katayama's use of
> INT or change the functions to use int like the UTF-8 functions do. So
> you're saying it should all be int? Do LPCSTR and LPCWSTR need to be
> changed to something else as well?

I'm GUESSING that it all needs to be changed to match the surrounding
definitions.
> > - No new tests to confirm the behaviour is correct (or mention of
> >
> > current tests succeeding)
>
> dlls/kernel32/tests/locale.c already has just as many UTF-7 tests as
> it has UTF-8 tests. What tests do we need for UTF-7 which are not
> needed for UTF-8?
>
> Thanks for your quick response,
>
> -Alex