Re: [2/2] msvcrt: Forward strftime() to wcsftime(). Take 2.

[Alexandre Julliard]

Dmitry Timoshkov  writes:
> - len = MultiByteToWideChar( CP_ACP, 0, format, -1, NULL, 0 );
> - if ((fmt = MSVCRT_malloc( len * sizeof(MSVCRT_wchar_t) )))
> - {
> -
>    MultiByteToWideChar( CP_ACP, 0, format, -1, fmt, len );
> -
> -
>    if ((len = MSVCRT_wcsftime( s, max, fmt, mstm )))
> -
>    {
> -
>        len = WideCharToMultiByte( CP_ACP, 0, s, len, str, max, NULL, NULL );
> -
>        if (len < max) str[len] = 0;
> -
>    }

The overflow handling still looks suspicious. It probably needs some
more test cases.

[2/2] msvcrt: Forward strftime() to wcsftime(). Take 2.

[Dmitry Timoshkov]

Alexandre Julliard  wrote:
> The overflow handling still looks suspicious. It probably needs some
> more test cases.

I'm probably missing something, why new attempt is marked as pending?

[2/2] msvcrt: Forward strftime() to wcsftime(). Take 2.

[Alexandre Julliard]

Dmitry Timoshkov  writes:
> Alexandre Julliard  wrote:
> > The overflow handling still looks suspicious. It probably needs some
> > more test cases.
>
> I'm probably missing something, why new attempt is marked as pending?

It's still broken. Consider what happens if there's no room for the
final null.

[2/2] msvcrt: Forward strftime() to wcsftime(). Take 2.

[Dmitry Timoshkov]

Alexandre Julliard  wrote:
> It's still broken. Consider what happens if there's no room for the
> final null.

Is there that kind of problem in the new tests?

[2/2] msvcrt: Forward strftime() to wcsftime(). Take 2.

[Alexandre Julliard]

Dmitry Timoshkov  writes:
> Alexandre Julliard  wrote:
> > It's still broken. Consider what happens if there's no room for the
> > final null.
>
> Is there that kind of problem in the new tests?

No but if the tests didn't catch that problem it's probably a sign that
they are not extensive enough.

[2/2] msvcrt: Forward strftime() to wcsftime(). Take 2.

[Dmitry Timoshkov]

Alexandre Julliard  wrote:
> > Is there that kind of problem in the new tests?
>
> No but if the tests didn't catch that problem it's probably a sign that
> they are not extensive enough.

New tests exercising any desirable behaviour could be added at any point,
it never was a reason to reject a test case before. If there is a need
for some specific test it's usually enough to suggest what you'd like
to see tested.

[2/2] msvcrt: Forward strftime() to wcsftime(). Take 2.

[Alexandre Julliard]

Dmitry Timoshkov  writes:
> Alexandre Julliard  wrote:
> > > Is there that kind of problem in the new tests?
> >
> > No but if the tests didn't catch that problem it's probably a sign that
> > they are not extensive enough.
>
> New tests exercising any desirable behaviour could be added at any point,
> it never was a reason to reject a test case before. If there is a need
> for some specific test it's usually enough to suggest what you'd like
> to see tested.

I didn't reject the test case. It's just pending until you get the code
right, which may require revisiting the tests.