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.