On Sun, Sep 29, 2013 at 11:10 PM, Daniel Jeliński <djelins...@gmail.com>wrote:
> 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
