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
