[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: Darren Kenny
Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v3 7/7] block/curl: code cleanup to comply with coding style
Date: Wed, 8 Nov 2017 14:50:38 +0000
User-agent: NeoMutt/20171027

On Wed, Nov 08, 2017 at 08:26:57AM -0600, Eric Blake wrote:
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

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.

If that is the purpose, maybe an #if 0 is more appropriate or #ifdef
DEBUG, or similar.

Isn't the purpose of styling to be consistent? As such should we not
be trying to use the multi-line style set out at the top of the file?

@@ -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
-    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).

Fair enough.

@@ -876,13 +880,13 @@ static void curl_setup_preadv(BlockDriverState
*bs, CURLAIOCB *acb)


-    // 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.



reply via email to

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