[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 <bunglehead at gmail.com 
> <mailto:bunglehead at gmail.com>>
>
>     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.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20130930/83a4ec72/attachment.html>