Re: [Qemu-block] [Qemu-devel] [PATCH v3 7/7] block/curl: code cleanup to comply with coding style

[Eric Blake]

On 11/08/2017 04:47 AM, Darren Kenny wrote:
> Hi Jeff,
> 
> While I'm relatively new to this community, I do have some comments
> about the styling in this file.
> 
> I don't see anything in the CODING_STYLE file that tells me I'm
> wrong here, but it's certainly possible...
> 
> More inline.
> 

>> -// #define DEBUG_CURL
>> -// #define DEBUG_VERBOSE

This is definitely against style (checkpatch.pl flags it),

>> +/*
>> + #define DEBUG_CURL
>> + #define DEBUG_VERBOSE
>> +*/
> 

while you are correct that this is not quite usual style.

> Is it not more common to use the style:
> 
>    /*
>     * text
>     */

But, since checkpatch.pl doesn't flag it, and since it is easier to
remove the leading and trailing /* and */ to enable the debug #defines
(compared to editing every single line of the comment), I don't see a
problem with the style chosen here.


>> @@ -235,7 +236,7 @@ static size_t curl_header_cb(void *ptr, size_t
>> size, size_t nmemb, void *opaque)
>> /* Called from curl_multi_do_locked, with s->mutex held.  */
>> static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void
>> *opaque)
>> {
>> -    CURLState *s = ((CURLState*)opaque);
>> +    CURLState *s = opaque;
> 
> Not sure about this one - while it may not be strictly necessary to
> do the casting, from a readability point of view it is helpful to
> see the cast - possibly with the outer brackets removed:
> 
>      CURLState *s = (CURLState*)opaque;

I _am_ sure about it. The cast from void* is pointless (this is C, not
C++), and this is one of the changes that Jeff specifically made in v3
that was not present in v2, because I requested that we lose the cast
(in general, we try to avoid as many casts as possible).

>> @@ -876,13 +880,13 @@ static void curl_setup_preadv(BlockDriverState
>> *bs, CURLAIOCB *acb)
>>
>>     qemu_mutex_lock(&s->mutex);
>>
>> -    // In case we have the requested data already (e.g. read-ahead),
>> -    // we can just call the callback and be done.
>> +    /* In case we have the requested data already (e.g. read-ahead),
>> +       we can just call the callback and be done. */
> 
> Please don't do a multi-line comment like this, it is much more
> obvious that the second line is a comment if you use the more normal
> format of as outlined earlier by me.

Indeed, while I don't mind the style used on the #define DEBUG_*, this
one could be touched up to be more idiomatic.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature