Re: [PATCH] kernel32: Implement CompareStringOrdinal. (try 2)

[Nikolay Sivov]

On 10/30/2012 01:43, Christian Costa wrote:
> Try 2:
> - Don't use CompareStringEx.
> - Add tests
>
> dlls/kernel32/kernel32.spec  |    1 +
>   dlls/kernel32/locale.c       |   28 ++++++++++++++++++++
>   dlls/kernel32/tests/locale.c |   60 +++++++++++++++++++++++++++++++++++++++++-
>   3 files changed, 88 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/kernel32/kernel32.spec b/dlls/kernel32/kernel32.spec
> index b7efa0f..0bd1adc 100644
> --- a/dlls/kernel32/kernel32.spec
> +++ b/dlls/kernel32/kernel32.spec
> @@ -201,6 +201,7 @@
>   @ stdcall CompareStringA(long long str long str long)
>   @ stdcall CompareStringW(long long wstr long wstr long)
>   @ stdcall CompareStringEx(wstr long wstr long wstr long ptr ptr long)
> +@ stdcall CompareStringOrdinal(wstr long wstr long long)
>   @ stdcall ConnectNamedPipe(long ptr)
>   @ stub ConsoleMenuControl
>   @ stub ConsoleSubst
> diff --git a/dlls/kernel32/locale.c b/dlls/kernel32/locale.c
> index c41442c..07eb688 100644
> --- a/dlls/kernel32/locale.c
> +++ b/dlls/kernel32/locale.c
> @@ -3047,6 +3047,34 @@ INT WINAPI CompareStringA(LCID lcid, DWORD flags,
>       return ret;
>   }
>
> +/******************************************************************************
> -
> -
>       CompareStringOrdinal    (KERNEL32.@)
> - */
>
> +INT WINAPI CompareStringOrdinal(const WCHAR *str1, INT len1, const WCHAR *str2, INT len2, BOOL ignore_case)
> +{
> - int ret, len;
> -
> - if (!str1 || !str2)
> - {
> -
>    SetLastError(ERROR_INVALID_PARAMETER);
> -
>    return 0;
> - }
> - if (len1 < 0) len1 = strlenW(str1);
> - if (len2 < 0) len2 = strlenW(str2);
> -
> - len = min(len1, len2);
> - if (ignore_case)
> -
>    ret = strncmpiW(str1, str2, len);
> - else
> -
>    ret = strncmpW(str1, str2, len);
> -
> - if ((ret < 0) || (!ret && (len1 < len2)))
> -
>    return CSTR_LESS_THAN;
> - else if ((ret > 0) || (!ret && (len1 > len2)))
> -
>    return CSTR_GREATER_THAN;
> - return CSTR_EQUAL;
>
> +}
> -

This is almost the same as your first try. You're testing a trivial case 
only - ASCII range.
I feel like it should behave more like memcmp for case insensitive 
comparison,
that's what documentation mildly suggests.

[PATCH] kernel32: Implement CompareStringOrdinal. (try 2)

[Christian Costa]

Le 30/10/2012 08:21, Nikolay Sivov a écrit :
> On 10/30/2012 01:43, Christian Costa wrote:
> > Try 2:
> > - Don't use CompareStringEx.
> > - Add tests
> >
> > dlls/kernel32/kernel32.spec  |    1 +
> >   dlls/kernel32/locale.c       |   28 ++++++++++++++++++++
> >   dlls/kernel32/tests/locale.c |   60 
> > +++++++++++++++++++++++++++++++++++++++++-
> >   3 files changed, 88 insertions(+), 1 deletion(-)
> >
> > diff --git a/dlls/kernel32/kernel32.spec b/dlls/kernel32/kernel32.spec
> > index b7efa0f..0bd1adc 100644
> > --- a/dlls/kernel32/kernel32.spec
> > +++ b/dlls/kernel32/kernel32.spec
> > @@ -201,6 +201,7 @@
> >   @ stdcall CompareStringA(long long str long str long)
> >   @ stdcall CompareStringW(long long wstr long wstr long)
> >   @ stdcall CompareStringEx(wstr long wstr long wstr long ptr ptr long)
> > +@ stdcall CompareStringOrdinal(wstr long wstr long long)
> >   @ stdcall ConnectNamedPipe(long ptr)
> >   @ stub ConsoleMenuControl
> >   @ stub ConsoleSubst
> > diff --git a/dlls/kernel32/locale.c b/dlls/kernel32/locale.c
> > index c41442c..07eb688 100644
> > --- a/dlls/kernel32/locale.c
> > +++ b/dlls/kernel32/locale.c
> > @@ -3047,6 +3047,34 @@ INT WINAPI CompareStringA(LCID lcid, DWORD flags,
> >       return ret;
> >   }
> > +/******************************************************************************
> > -
> > -
> >       CompareStringOrdinal    (KERNEL32.@)
> > - */
> >
> > +INT WINAPI CompareStringOrdinal(const WCHAR *str1, INT len1, const 
> > WCHAR *str2, INT len2, BOOL ignore_case)
> > +{
> > - int ret, len;
> > -
> > - if (!str1 || !str2)
> > - {
> > -
> >    SetLastError(ERROR_INVALID_PARAMETER);
> > -
> >    return 0;
> > - }
> > - if (len1 < 0) len1 = strlenW(str1);
> > - if (len2 < 0) len2 = strlenW(str2);
> > -
> > - len = min(len1, len2);
> > - if (ignore_case)
> > -
> >    ret = strncmpiW(str1, str2, len);
> > - else
> > -
> >    ret = strncmpW(str1, str2, len);
> > -
> > - if ((ret < 0) || (!ret && (len1 < len2)))
> > -
> >    return CSTR_LESS_THAN;
> > - else if ((ret > 0) || (!ret && (len1 > len2)))
> > -
> >    return CSTR_GREATER_THAN;
> > - return CSTR_EQUAL;
> >
> > +}
> > -
>
> This is almost the same as your first try. You're testing a trivial 
> case only - ASCII range.
> I feel like it should behave more like memcmp for case insensitive 
> comparison,
> that's what documentation mildly suggests.

Well, It's not the same implementation. It's ordinal now. I have test 
for that.
And how can memcmp be used for insensitive comparison?
Do you have a concrete example that does not work with this implementation?

[PATCH] kernel32: Implement CompareStringOrdinal. (try 2)

[Hans Leidekker]

On Tue, 2012-10-30 at 08:42 +0100, Christian Costa wrote:
> > This is almost the same as your first try. You're testing a trivial 
> > case only - ASCII range.
> > I feel like it should behave more like memcmp for case insensitive 
> > comparison,
> > that's what documentation mildly suggests.
>
> Well, It's not the same implementation. It's ordinal now. I have test 
> for that.
> And how can memcmp be used for insensitive comparison?

Have a look at memicmpW.
> Do you have a concrete example that does not work with this implementation

How about CompareStringOrdinal( "a\0a", 3, "a\0b", 3 )?

[PATCH] kernel32: Implement CompareStringOrdinal. (try 2)

[Christian Costa]

Le 30/10/2012 09:18, Hans Leidekker a écrit :
> On Tue, 2012-10-30 at 08:42 +0100, Christian Costa wrote:
> > > This is almost the same as your first try. You're testing a trivial
> > > case only - ASCII range.
> > > I feel like it should behave more like memcmp for case insensitive
> > > comparison,
> > > that's what documentation mildly suggests.
> >
> > Well, It's not the same implementation. It's ordinal now. I have test
> > for that.
> > And how can memcmp be used for insensitive comparison?
>
> Have a look at memicmpW.

Just by curiosity. What the difference with strncmpW?
> > Do you have a concrete example that does not work with this implementation
>
> How about CompareStringOrdinal( "a\0a", 3, "a\0b", 3 )?

There is a similar test in the patch:

WCHAR test1[] = { 't','e','s','t',0 };
     WCHAR test3[] = { 't','e','s','t','3',0 };

ret = pCompareStringOrdinal(test1, 5, test3, 5, TRUE);
     ok(ret == CSTR_LESS_THAN, "Got %u, expected %u\n", ret, CSTR_LESS_THAN);

[PATCH] kernel32: Implement CompareStringOrdinal. (try 2)

[Hans Leidekker]

On Tue, 2012-10-30 at 09:50 +0100, Christian Costa wrote:
> Just by curiosity. What the difference with strncmpW?

strncmp tests the first string for null termination. You should really
determine that by yourself though :-)
> > > Do you have a concrete example that does not work with this implementation
> >
> > How about CompareStringOrdinal( "a\0a", 3, "a\0b", 3 )?
>
> There is a similar test in the patch:
>  WCHAR test1[] = { 't','e','s','t',0 };
>  WCHAR test3[] = { 't','e','s','t','3',0 };
>
>  ret = pCompareStringOrdinal(test1, 5, test3, 5, TRUE);
>  ok(ret == CSTR_LESS_THAN, "Got %u, expected %u\n", ret, CSTR_LESS_THAN);

It's not the same test. The nulls are not at the same index and you're not
asking CompareStringOrdinal to read past a null.

[PATCH] kernel32: Implement CompareStringOrdinal. (try 2)

[Christian Costa]

2012/10/30 Hans Leidekker
> On Tue, 2012-10-30 at 09:50 +0100, Christian Costa wrote:
> > Just by curiosity. What the difference with strncmpW?
>
> strncmp tests the first string for null termination. You should really
> determine that by yourself though :-)
> > > > Do you have a concrete example that does not work with this
>
> implementation
> > > How about CompareStringOrdinal( "a\0a", 3, "a\0b", 3 )?
> >
> > There is a similar test in the patch:
> >  WCHAR test1[] = { 't','e','s','t',0 };
> >  WCHAR test3[] = { 't','e','s','t','3',0 };
> >
> >  ret = pCompareStringOrdinal(test1, 5, test3, 5, TRUE);
> >  ok(ret == CSTR_LESS_THAN, "Got %u, expected %u\n", ret,
>
> CSTR_LESS_THAN);
>
> It's not the same test. The nulls are not at the same index and you're not
> asking CompareStringOrdinal to read past a null.
>
> I just wanted to compare 0 with 3 but you're right, I'll add such a test.

[PATCH] kernel32: Implement CompareStringOrdinal. (try 2)

[Nikolay Sivov]

On 10/30/2012 10:42, Christian Costa wrote:
> Le 30/10/2012 08:21, Nikolay Sivov a écrit :
> > On 10/30/2012 01:43, Christian Costa wrote:
> > > Try 2:
> > > - Don't use CompareStringEx.
> > > - Add tests
> > >
> > > dlls/kernel32/kernel32.spec  |    1 +
> > >   dlls/kernel32/locale.c       |   28 ++++++++++++++++++++
> > >   dlls/kernel32/tests/locale.c |   60 
> > > +++++++++++++++++++++++++++++++++++++++++-
> > >   3 files changed, 88 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/dlls/kernel32/kernel32.spec b/dlls/kernel32/kernel32.spec
> > > index b7efa0f..0bd1adc 100644
> > > --- a/dlls/kernel32/kernel32.spec
> > > +++ b/dlls/kernel32/kernel32.spec
> > > @@ -201,6 +201,7 @@
> > >   @ stdcall CompareStringA(long long str long str long)
> > >   @ stdcall CompareStringW(long long wstr long wstr long)
> > >   @ stdcall CompareStringEx(wstr long wstr long wstr long ptr ptr long)
> > > +@ stdcall CompareStringOrdinal(wstr long wstr long long)
> > >   @ stdcall ConnectNamedPipe(long ptr)
> > >   @ stub ConsoleMenuControl
> > >   @ stub ConsoleSubst
> > > diff --git a/dlls/kernel32/locale.c b/dlls/kernel32/locale.c
> > > index c41442c..07eb688 100644
> > > --- a/dlls/kernel32/locale.c
> > > +++ b/dlls/kernel32/locale.c
> > > @@ -3047,6 +3047,34 @@ INT WINAPI CompareStringA(LCID lcid, DWORD 
> > > flags,
> > >       return ret;
> > >   }
> > > +/******************************************************************************
> > > -
> > > -
> > >       CompareStringOrdinal    (KERNEL32.@)
> > > - */
> > >
> > > +INT WINAPI CompareStringOrdinal(const WCHAR *str1, INT len1, const 
> > > WCHAR *str2, INT len2, BOOL ignore_case)
> > > +{
> > > - int ret, len;
> > > -
> > > - if (!str1 || !str2)
> > > - {
> > > -
> > >    SetLastError(ERROR_INVALID_PARAMETER);
> > > -
> > >    return 0;
> > > - }
> > > - if (len1 < 0) len1 = strlenW(str1);
> > > - if (len2 < 0) len2 = strlenW(str2);
> > > -
> > > - len = min(len1, len2);
> > > - if (ignore_case)
> > > -
> > >    ret = strncmpiW(str1, str2, len);
> > > - else
> > > -
> > >    ret = strncmpW(str1, str2, len);
> > > -
> > > - if ((ret < 0) || (!ret && (len1 < len2)))
> > > -
> > >    return CSTR_LESS_THAN;
> > > - else if ((ret > 0) || (!ret && (len1 > len2)))
> > > -
> > >    return CSTR_GREATER_THAN;
> > > - return CSTR_EQUAL;
> > >
> > > +}
> > > -
> >
> > This is almost the same as your first try. You're testing a trivial 
> > case only - ASCII range.
> > I feel like it should behave more like memcmp for case insensitive 
> > comparison,
> > that's what documentation mildly suggests.
>
> Well, It's not the same implementation. It's ordinal now. I have test 
> for that.
> And how can memcmp be used for insensitive comparison?

Yeah, sorry for a noise, I thought strncmpW is still too smart for some 
reason. And I meant memcmp for sensitive case of course,
but that's what strncmpW already is.
> Do you have a concrete example that does not work with this 
> implementation?

No, it would be nice to have some tests outside ascii range, but this 
could be added later.