On 9/30/2013 10:10, Daniel Jeliński wrote:
2013/9/30 Nikolay Sivov <bunglehead@gmail.com mailto:bunglehead@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.