Re: [2/3] kernel32/tests: test CopyFileEx callback and cancellation (resend)

[Nikolay Sivov]

On 9/30/2013 00:51, Daniel Jeliński wrote:
> +struct progress_list {
> - const DWORD progress_retval_init;  /* value to return from progress routine */
> - const BOOL cancel_init;            /* value to set Cancel flag to */
> - const DWORD progress_retval_end;   /* value to return from progress routine */
> - const BOOL cancel_end;             /* value to set Cancel flag to */
> - const DWORD progress_count;        /* number of times progress is invoked */
> - const BOOL copy_retval;            /* expected CopyFileEx result */
> - const DWORD lastError;             /* expected CopyFileEx error code */
>
> +} ;

I don't see a point making them 'const'.
> +static DWORD WINAPI progress(LARGE_INTEGER TotalFileSize,
> -
>    LARGE_INTEGER TotalBytesTransferred,
> -
>    LARGE_INTEGER StreamSize,
> -
>    LARGE_INTEGER StreamBytesTransferred,
> -
>    DWORD dwStreamNumber,
> -
>    DWORD dwCallbackReason,
> -
>    HANDLE hSourceFile,
> -
>    HANDLE hDestinationFile,
> -
>    LPVOID lpData)
>
> +{
> - progressInvoked++;

Please pass all globals as context data with lpData, and please use 
'void*' instead of LPVOID.

[2/3] kernel32/tests: test CopyFileEx callback and cancellation (resend)

[Daniel Jeliński]

2013/9/30 Nikolay Sivov
> On 9/30/2013 00:51, Daniel Jeliński wrote:
>
> +struct progress_list {
> > - const DWORD progress_retval_init;  /* value to return from progress
> >
> > routine */
> > - const BOOL cancel_init;            /* value to set Cancel flag to */
> > - const DWORD progress_retval_end;   /* value to return from progress
> >
> > routine */
> > - const BOOL cancel_end;             /* value to set Cancel flag to */
> > - const DWORD progress_count;        /* number of times progress is
> >
> > invoked */
> > - const BOOL copy_retval;            /* expected CopyFileEx result */
> > - const DWORD lastError;             /* expected CopyFileEx error code
> >
> > */
> > +} ;
>
> I don't see a point making them 'const'.

I'm matching the formatting of existing code:
http://source.winehq.org/source/dlls/kernel32/tests/file.c#L65
Also, what's the point of not making them const?

+static DWORD WINAPI progress(LARGE_INTEGER TotalFileSize,
> > -
> >    LARGE_INTEGER TotalBytesTransferred,
> > -
> >    LARGE_INTEGER StreamSize,
> > -
> >    LARGE_INTEGER StreamBytesTransferred,
> > -
> >    DWORD dwStreamNumber,
> > -
> >    DWORD dwCallbackReason,
> > -
> >    HANDLE hSourceFile,
> > -
> >    HANDLE hDestinationFile,
> > -
> >    LPVOID lpData)
> >
> > +{
> > - progressInvoked++;
>
> Please pass all globals as context data with lpData, and please use
> 'void*' instead of LPVOID.

Good point about lpData. Still, does that make the patch invalid? Why
didn't you mention that on the first review?
About LPVOID - I'm matching the headers:
http://source.winehq.org/source/include/winbase.h#L910
for the third patch:
http://source.winehq.org/source/include/winbase.h#L1018

Also, any comments on patch 3?

[2/3] kernel32/tests: test CopyFileEx callback and cancellation (resend)

[Juan Lang]

On Sun, Sep 29, 2013 at 11:10 PM, Daniel Jeliński wrote:
> 2013/9/30 Nikolay Sivov
> > On 9/30/2013 00:51, Daniel Jeliński wrote:
> >
> > +struct progress_list {
> > > - const DWORD progress_retval_init;  /* value to return from progress
> > >
> > > routine */
> > > - const BOOL cancel_init;            /* value to set Cancel flag to */
> > > - const DWORD progress_retval_end;   /* value to return from progress
> > >
> > > routine */
> > > - const BOOL cancel_end;             /* value to set Cancel flag to */
> > > - const DWORD progress_count;        /* number of times progress is
> > >
> > > invoked */
> > > - const BOOL copy_retval;            /* expected CopyFileEx result */
> > > - const DWORD lastError;             /* expected CopyFileEx error
> > >
> > > code */
> > > +} ;
> >
> > I don't see a point making them 'const'.
>
> I'm matching the formatting of existing code:
> http://source.winehq.org/source/dlls/kernel32/tests/file.c#L65
> Also, what's the point of not making them const?

It's a little strange, stylewise. More consistent with C++ style would be
to make the entire struct constant. But in a test, we often eschew with
such things if they distract, and here I think they do.
> +static DWORD WINAPI progress(LARGE_INTEGER TotalFileSize,
> > > -
> > >    LARGE_INTEGER TotalBytesTransferred,
> > > -
> > >    LARGE_INTEGER StreamSize,
> > > -
> > >    LARGE_INTEGER StreamBytesTransferred,
> > > -
> > >    DWORD dwStreamNumber,
> > > -
> > >    DWORD dwCallbackReason,
> > > -
> > >    HANDLE hSourceFile,
> > > -
> > >    HANDLE hDestinationFile,
> > > -
> > >    LPVOID lpData)
> > >
> > > +{
> > > - progressInvoked++;
> >
> > Please pass all globals as context data with lpData, and please use
> > 'void*' instead of LPVOID.
>
> Good point about lpData. Still, does that make the patch invalid? Why
> didn't you mention that on the first review?
> About LPVOID - I'm matching the headers:
> http://source.winehq.org/source/include/winbase.h#L910
> for the third patch:
> http://source.winehq.org/source/include/winbase.h#L1018

LPVOID is just an uglier #define of void*. It's easier to read as void*, so
please use that instead.
--Juan

[2/3] kernel32/tests: test CopyFileEx callback and cancellation (resend)

[Nikolay Sivov]

On 9/30/2013 10:10, Daniel Jeliński wrote:
> 2013/9/30 Nikolay Sivov <[e-mail] 
> >
> On 9/30/2013 00:51, Daniel Jeliński wrote:
>
>
>     +struct progress_list {
>     +    const DWORD progress_retval_init;  /* value to return
>     from progress routine */
>     +    const BOOL cancel_init;            /* value to set Cancel
>     flag to */
>     +    const DWORD progress_retval_end;   /* value to return
>     from progress routine */
>     +    const BOOL cancel_end;             /* value to set Cancel
>     flag to */
>     +    const DWORD progress_count;        /* number of times
>     progress is invoked */
>     +    const BOOL copy_retval;            /* expected CopyFileEx
>     result */
>     +    const DWORD lastError;             /* expected CopyFileEx
>     error code */
>     +} ;
>
>  I don't see a point making them 'const'.
>
> I'm matching the formatting of existing code:
> http://source.winehq.org/source/dlls/kernel32/tests/file.c#L65
> Also, what's the point of not making them const?
>     +static DWORD WINAPI progress(LARGE_INTEGER TotalFileSize,
>     +        LARGE_INTEGER TotalBytesTransferred,
>     +        LARGE_INTEGER StreamSize,
>     +        LARGE_INTEGER StreamBytesTransferred,
>     +        DWORD dwStreamNumber,
>     +        DWORD dwCallbackReason,
>     +        HANDLE hSourceFile,
>     +        HANDLE hDestinationFile,
>     +        LPVOID lpData)
>     +{
>     +    progressInvoked++;
>
> Please pass all globals as context data with lpData, and please
> use 'void*' instead of LPVOID.
>
> Good point about lpData. Still, does that make the patch invalid?

It's not black or white, I mentioned what will be nice to do about it to 
make it more compact and self-contained.
It doesn't mean it's invalid, it's just an obvious thing that will make 
it better.
> Why didn't you mention that on the first review?

Because sometimes I stop reading further after I see major problems.
> Also, any comments on patch 3?

Same thing, you could easily pack everything to a single struct and pass 
it using this context pointer.
It could also be made more compact using a single helper to compar 
COPYFILE2_MESSAGE value,
instead of duplicating it for every message type.