[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric Blake
Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v3 7/7] block/curl: code cleanup to comply with coding style
Date: Wed, 8 Nov 2017 08:26:57 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0

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

Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]