Re: [PATCH] kernel32/tests/fiber: Add tests for fiber-local storage.

[André Hentschel]

Hi,

comments inline

Am 08.07.2016 um 00:16 schrieb John Sheu:
> +static void test_FiberLocalStorageCallback(PFLS_CALLBACK_FUNCTION cbfunc) {
> - DWORD fls;
> - BOOL ret;
> - PVOID val, val2;
> -
> - if (!pFlsAlloc || !pFlsSetValue || !pFlsGetValue || !pFlsFree)
> - {
> -
>    win_skip( "Fiber Local Storage not supported\n" );
> -
>    return;
> - }
> -
> - /* Test that the callback is executed */
> - SetLastError( 0xdeadbeef );
> - cbCount = 0;
> - fls = pFlsAlloc( cbfunc );
> - ok( fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %u\n", GetLastError() );
> -
> - SetLastError( 0xdeadbeef );
> - val = (PVOID) 0x1587;
> - fls_value_to_set = val;
> - ret = pFlsSetValue( fls, val );
> - ok(ret, "FlsSetValue failed with error %u\n", GetLastError() );
> -
> - val2 = pFlsGetValue( fls );
> - ok(val == val2, "FlsGetValue returned %p, expected %p\n", val2, val);
> -
> - SetLastError( 0xdeadbeef );
> - ret = pFlsFree( fls );
> - ok(ret, "FlsFree failed with error %u\n", GetLastError() );
> - todo_wine
> - {
> -
>    ok( cbCount == 1, "Wrong callback count: %d\n", cbCount );
> - }

Please make those one-liners, like
todo_wine ok(...);
and the patch should be fine then
> -
> - /* Test that callback is not executed if value is NULL */
> - SetLastError( 0xdeadbeef );
> - cbCount = 0;
> - fls = pFlsAlloc( cbfunc );
> - ok( fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %u\n", GetLastError() );
> -
> - SetLastError( 0xdeadbeef );
> - ret = pFlsSetValue( fls, NULL );
> - ok( ret, "FlsSetValue failed with error %u\n", GetLastError() );
> -
> - SetLastError( 0xdeadbeef );
> - pFlsFree( fls );
> - ok( ret, "FlsFree failed with error %u\n", GetLastError() );
> - ok( cbCount == 0, "Wrong callback count: %d\n", cbCount );
>
> +}

[PATCH] kernel32/tests/fiber: Add tests for fiber-local storage.

[Nikolay Sivov]

On 10.07.2016 20:31, André Hentschel wrote:
> Hi,
>
> comments inline
>
> Am 08.07.2016 um 00:16 schrieb John Sheu:
> > +static void test_FiberLocalStorageCallback(PFLS_CALLBACK_FUNCTION cbfunc) {
> > - DWORD fls;
> > - BOOL ret;
> > - PVOID val, val2;
> > -
> > - if (!pFlsAlloc || !pFlsSetValue || !pFlsGetValue || !pFlsFree)
> > - {
> > -
> >    win_skip( "Fiber Local Storage not supported\n" );
> > -
> >    return;
> > - }
> > -
> > - /* Test that the callback is executed */
> > - SetLastError( 0xdeadbeef );
> > - cbCount = 0;
> > - fls = pFlsAlloc( cbfunc );
> > - ok( fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %u\n", GetLastError() );
> > -
> > - SetLastError( 0xdeadbeef );
> > - val = (PVOID) 0x1587;
> > - fls_value_to_set = val;
> > - ret = pFlsSetValue( fls, val );
> > - ok(ret, "FlsSetValue failed with error %u\n", GetLastError() );
> > -
> > - val2 = pFlsGetValue( fls );
> > - ok(val == val2, "FlsGetValue returned %p, expected %p\n", val2, val);
> > -
> > - SetLastError( 0xdeadbeef );
> > - ret = pFlsFree( fls );
> > - ok(ret, "FlsFree failed with error %u\n", GetLastError() );
> > - todo_wine
> > - {
> > -
> >    ok( cbCount == 1, "Wrong callback count: %d\n", cbCount );
> > - }
>
> Please make those one-liners, like
> todo_wine ok(...);
> and the patch should be fine then

How is this a stopper?
> > -
> > - /* Test that callback is not executed if value is NULL */
> > - SetLastError( 0xdeadbeef );
> > - cbCount = 0;
> > - fls = pFlsAlloc( cbfunc );
> > - ok( fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %u\n", GetLastError() );
> > -
> > - SetLastError( 0xdeadbeef );
> > - ret = pFlsSetValue( fls, NULL );
> > - ok( ret, "FlsSetValue failed with error %u\n", GetLastError() );
> > -
> > - SetLastError( 0xdeadbeef );
> > - pFlsFree( fls );
> > - ok( ret, "FlsFree failed with error %u\n", GetLastError() );
> > - ok( cbCount == 0, "Wrong callback count: %d\n", cbCount );
> >
> > +}

[PATCH] kernel32/tests/fiber: Add tests for fiber-local storage.

[André Hentschel]

Am 10.07.2016 um 19:38 schrieb Nikolay Sivov:
> On 10.07.2016 20:31, André Hentschel wrote:
> > Hi,
> >
> > comments inline
> >
> > Am 08.07.2016 um 00:16 schrieb John Sheu:
> > > +static void test_FiberLocalStorageCallback(PFLS_CALLBACK_FUNCTION cbfunc) {
> > > - DWORD fls;
> > > - BOOL ret;
> > > - PVOID val, val2;
> > > -
> > > - if (!pFlsAlloc || !pFlsSetValue || !pFlsGetValue || !pFlsFree)
> > > - {
> > > -
> > >    win_skip( "Fiber Local Storage not supported\n" );
> > > -
> > >    return;
> > > - }
> > > -
> > > - /* Test that the callback is executed */
> > > - SetLastError( 0xdeadbeef );
> > > - cbCount = 0;
> > > - fls = pFlsAlloc( cbfunc );
> > > - ok( fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %u\n", GetLastError() );
> > > -
> > > - SetLastError( 0xdeadbeef );
> > > - val = (PVOID) 0x1587;
> > > - fls_value_to_set = val;
> > > - ret = pFlsSetValue( fls, val );
> > > - ok(ret, "FlsSetValue failed with error %u\n", GetLastError() );
> > > -
> > > - val2 = pFlsGetValue( fls );
> > > - ok(val == val2, "FlsGetValue returned %p, expected %p\n", val2, val);
> > > -
> > > - SetLastError( 0xdeadbeef );
> > > - ret = pFlsFree( fls );
> > > - ok(ret, "FlsFree failed with error %u\n", GetLastError() );
> > > - todo_wine
> > > - {
> > > -
> > >    ok( cbCount == 1, "Wrong callback count: %d\n", cbCount );
> > > - }
> >
> > Please make those one-liners, like
> > todo_wine ok(...);
> > and the patch should be fine then
>
> How is this a stopper?

It's not directly a stopper.
But I personally really don't like it that way and it somehow prevents me from a sign-off. And I think I'll add myself as a Maintainer for that file, so the style should fit somewhat.

[PATCH] kernel32/tests/fiber: Add tests for fiber-local storage.

[Sebastian Lackner]

On 10.07.2016 19:51, André Hentschel wrote:
> Am 10.07.2016 um 19:38 schrieb Nikolay Sivov:
> > On 10.07.2016 20:31, André Hentschel wrote:
> > > Hi,
> > >
> > > comments inline
> > >
> > > Am 08.07.2016 um 00:16 schrieb John Sheu:
> > > > +static void test_FiberLocalStorageCallback(PFLS_CALLBACK_FUNCTION cbfunc) {
> > > > - DWORD fls;
> > > > - BOOL ret;
> > > > - PVOID val, val2;
> > > > -
> > > > - if (!pFlsAlloc || !pFlsSetValue || !pFlsGetValue || !pFlsFree)
> > > > - {
> > > > -
> > > >    win_skip( "Fiber Local Storage not supported\n" );
> > > > -
> > > >    return;
> > > > - }
> > > > -
> > > > - /* Test that the callback is executed */
> > > > - SetLastError( 0xdeadbeef );
> > > > - cbCount = 0;
> > > > - fls = pFlsAlloc( cbfunc );
> > > > - ok( fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %u\n", GetLastError() );
> > > > -
> > > > - SetLastError( 0xdeadbeef );
> > > > - val = (PVOID) 0x1587;
> > > > - fls_value_to_set = val;
> > > > - ret = pFlsSetValue( fls, val );
> > > > - ok(ret, "FlsSetValue failed with error %u\n", GetLastError() );
> > > > -
> > > > - val2 = pFlsGetValue( fls );
> > > > - ok(val == val2, "FlsGetValue returned %p, expected %p\n", val2, val);
> > > > -
> > > > - SetLastError( 0xdeadbeef );
> > > > - ret = pFlsFree( fls );
> > > > - ok(ret, "FlsFree failed with error %u\n", GetLastError() );
> > > > - todo_wine
> > > > - {
> > > > -
> > > >    ok( cbCount == 1, "Wrong callback count: %d\n", cbCount );
> > > > - }
> > >
> > > Please make those one-liners, like
> > > todo_wine ok(...);
> > > and the patch should be fine then
> >
> > How is this a stopper?
>
> It's not directly a stopper.
> But I personally really don't like it that way and it somehow prevents me from a sign-off. And I think I'll add myself as a Maintainer for that file, so the style should fit somewhat.

Its up to you of course, but please note that you can also send a fixed version yourself.
Just make sure to add a From: header such that the original author does not get lost.