[prev in list] [next in list] [prev in thread] [next in thread]
List: wine-devel
Subject: Re: [2/3] kernel32/tests: test CopyFileEx callback and cancellation (resend)
From: Nikolay Sivov <bunglehead () gmail ! com>
Date: 2013-09-30 15:21:40
Message-ID: 52499704.5000801 () gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
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.
[Attachment #5 (text/html)]
<html>
<head>
<meta content="text/html; charset=ISO-8859-2"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 9/30/2013 10:10, Daniel Jeliński
wrote:<br>
</div>
<blockquote
cite="mid:CAMrH03+HkkJ_60VB5UCJ-mHkmCxm_fF-doyCJ9NAC621v3ddyw@mail.gmail.com"
type="cite">
<div dir="ltr">2013/9/30 Nikolay Sivov <span dir="ltr"><<a
moz-do-not-send="true" href="mailto:bunglehead@gmail.com"
target="_blank">bunglehead@gmail.com</a>></span><br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
On 9/30/2013 00:51, Daniel Jeliński wrote:<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+struct progress_list {<br>
+ const DWORD progress_retval_init; /* value to return
from progress routine */<br>
+ const BOOL cancel_init; /* value to set
Cancel flag to */<br>
+ const DWORD progress_retval_end; /* value to return
from progress routine */<br>
+ const BOOL cancel_end; /* value to set
Cancel flag to */<br>
+ const DWORD progress_count; /* number of times
progress is invoked */<br>
+ const BOOL copy_retval; /* expected
CopyFileEx result */<br>
+ const DWORD lastError; /* expected
CopyFileEx error code */<br>
+} ;<br>
</blockquote>
I don't see a point making them 'const'.<br>
</blockquote>
<div>I'm matching the formatting of existing code:</div>
<div><a moz-do-not-send="true"
href="http://source.winehq.org/source/dlls/kernel32/tests/file.c#L65">http://source.winehq.org/source/dlls/kernel32/tests/file.c#L65</a></div>
<div>Also, what's the point of not making them const?</div>
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+static DWORD WINAPI progress(LARGE_INTEGER TotalFileSize,<br>
+ LARGE_INTEGER TotalBytesTransferred,<br>
+ LARGE_INTEGER StreamSize,<br>
+ LARGE_INTEGER StreamBytesTransferred,<br>
+ DWORD dwStreamNumber,<br>
+ DWORD dwCallbackReason,<br>
+ HANDLE hSourceFile,<br>
+ HANDLE hDestinationFile,<br>
+ LPVOID lpData)<br>
+{<br>
+ progressInvoked++;<br>
</blockquote>
Please pass all globals as context data with lpData, and
please use 'void*' instead of LPVOID.<br>
</blockquote>
Good point about lpData. Still, does that make the patch
invalid?</div>
</blockquote>
It's not black or white, I mentioned what will be nice to do about
it to make it more compact and self-contained.<br>
It doesn't mean it's invalid, it's just an obvious thing that will
make it better.<br>
<blockquote
cite="mid:CAMrH03+HkkJ_60VB5UCJ-mHkmCxm_fF-doyCJ9NAC621v3ddyw@mail.gmail.com"
type="cite">
<div dir="ltr"> Why didn't you mention that on the first review?</div>
</blockquote>
Because sometimes I stop reading further after I see major problems.<br>
<blockquote
cite="mid:CAMrH03+HkkJ_60VB5UCJ-mHkmCxm_fF-doyCJ9NAC621v3ddyw@mail.gmail.com"
type="cite">
<div dir="ltr"><br>
<div class="gmail_extra"><br>
</div>
<div class="gmail_extra">Also, any comments on patch 3?</div>
<div class="gmail_extra"><br>
</div>
</div>
</blockquote>
Same thing, you could easily pack everything to a single struct and
pass it using this context pointer.<br>
It could also be made more compact using a single helper to compar
COPYFILE2_MESSAGE value,<br>
instead of duplicating it for every message type.<br>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic