Re: [2/3] kernel32/tests: test CopyFileEx callback and cancellation (resend)
2013/9/30 Nikolay Sivov bungleh...@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? 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?
Re: [2/3] kernel32/tests: test CopyFileEx callback and cancellation (resend)
On Sun, Sep 29, 2013 at 11:10 PM, Daniel Jeliński djelins...@gmail.comwrote:
2013/9/30 Nikolay Sivov bungleh...@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?
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
Re: [2/3] kernel32/tests: test CopyFileEx callback and cancellation (resend)
On 9/30/2013 10:10, Daniel Jeliński wrote:
2013/9/30 Nikolay Sivov bungleh...@gmail.com
mailto:bungleh...@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.
Re: [2/3] kernel32/tests: test CopyFileEx callback and cancellation (resend)
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.
