Libguestfs - Feb 2023 - [libnbd PATCH v3 01/29] use space consistently in function and function-like macro invocations

On 2/15/23 20:48, Eric Blake wrote:
> On Wed, Feb 15, 2023 at 03:11:30PM +0100, Laszlo Ersek wrote:
>> We intend to place a space character between the function designator
and
>> the opening parenthesis in a function call. We've been executing on
that
>> mostly consistently; fix the few exceptions now.
>>
>> The same convention should be applied to the invocations of
function-like
>> macros, and to instances of "__attribute__ ((attr))". (The
latter is
>> exemplified, although not consistently, by the GCC manual.) Implement
>> this, by inserting the necessary spaces.
>>
>> Furthermore, some compiler attributes take multiple parameters, such as
>> "nonnull". The GCC manual clearly shows that arguments passed
to such
>> attributes may be separated with ", " as well, not just
",". Use the
>> former, as it's more readable.
>>
>> Finally, the C standard calls "defined" -- as in "#if
defined identifier"
>> and (equivalently) "#if defined (identifier)" -- a unary
preprocessing
>> operator. We can spell the parenthesized form as "defined
(identifier)"
>> rather than "defined(identifier)", so choose the former.
>>
>> I collected the locations possibly missing spaces with:
>>
>>   git grep -EHn '\<[a-zA-Z0-9_]+\(' -- '*.c'
'*.h'
>>
>> and then manually updated each as necessary.
>>
>> I didn't change occurrences in comments, except where the comment
clearly
>> indicated copying and pasting an expression into new code.
>>
>> "git show -w" outputs nothing for this patch.
> 
> Which would not remain the case if we reflow a long line made longer
> (see [1] below).
> 
>>
>> The test suite passes.
>>
>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>> ---
> 
>> +++ b/lib/internal.h
>> @@ -46,7 +46,7 @@
>>   * debug and error handling code out of hot paths, making the hot path
>>   * into common functions use less instruction cache.
>>   */
>> -#if defined(__GNUC__)
>> +#if defined (__GNUC__)
> 
> In my experience with GNU code (which this is not), the style I've
> seen there is to omit () whenever possible, as in:
> 
> #if defined __GNUC__
> 
> or even
> 
> #ifdef __GNUC__
> 
> ...
> 
>> +++ b/common/include/checked-overflow.h
>> @@ -46,7 +46,7 @@
>>  #ifndef NBDKIT_CHECKED_OVERFLOW_H
>>  #define NBDKIT_CHECKED_OVERFLOW_H
>>  
>> -#if !defined(__GNUC__) && !defined(__clang__)
>> +#if !defined (__GNUC__) && !defined (__clang__)
> 
> ...obviously, the shorter #ifdef version can't be used here, but it
> could still be shortened to:
> 
> #if !defined __GNUC__ && !defined __clang__
> 
>> @@ -173,10 +173,10 @@
>>   *
>>   * The expression "x" is not evaluated, unless it has
variably modified type.
>>   */
>> -#define STATIC_ASSERT_UNSIGNED_INT(x)                                 
\
>> -  do {                                                                
\
>> -    typedef char NBDKIT_UNIQUE_NAME(_x_has_uint_type)[(typeof (x))-1
> 0 ? 1 : -1] \
>> -      __attribute__((__unused__));                                    
\
>> +#define STATIC_ASSERT_UNSIGNED_INT(x)                                 
\
>> +  do {                                                                
\
>> +    typedef char NBDKIT_UNIQUE_NAME (_x_has_uint_type)[(typeof (x))-1
> 0 ? 1 : -1] \
>> +      __attribute__ ((__unused__));                                   
\
> 
> [1] This change widened out beyond 80 columns.  Is it worth splitting
> that typedef line in two, perhaps as:
> 
>     typedef char NBDKIT_UNIQUE_NAME (_x_has_uint_type)               \
>         [(typeof (x))-1 > 0 ? 1 : -1] __attribute__ ((__unused__));  \
> 
> or does that make it look worse?  At any rate, even if we do want to
> reflow the line to be shorter, you have to consider the commit message
> blurb about 'git show -w'.
This patch concerns itself with one thing only: the space character
between the function designator (or macro name) and the paren that opens
the argument list. Everything else is out of scope for the patch.

In the particular case, the original NBDKIT_UNIQUE_NAME line was already
84 characters long; in fact that original line, when introduced, broke
the alignment of the backslashes at the right hand side. That state
stems from nbdkit (not libnbd) commit cf2b6297646a
("common/include/unique-name.h: Rename UNIQUE_NAME macro",
2022-01-11).
It was later ported to libnbd in commit 485f5ddf2d48
("common/include/unique-name.h: Rename UNIQUE_NAME macro",
2022-02-23).

So, this patch highlights another pre-existent task, and creates a new one:

- the overlong lines from the above-noted commits should be cleaned up

- the whitespace updates from the present patch should be reflected back
to nbdkit.

I was aware of both of those tasks, it's just that my cleanup stack
simply couldn't accommodate further recursions. I was already cleaning
up a thing for a cleanup that I needed for another cleanup. I couldn't
nest them any deeper, I had to stop the scope creep somewhere.

More generally, we should probably invent a way to avoid porting such
utility code back and forth between libnbd and nbdkit. For example,
libguestfs, guestfs-tools, and virt-v2v share the "common" submodule.
> 
>> +++ b/common/utils/string-vector.h
>> @@ -37,6 +37,6 @@
>>  
>>  #include "vector.h"
>>  
>> -DEFINE_VECTOR_TYPE(string_vector, char *);
>> +DEFINE_VECTOR_TYPE (string_vector, char *);
> 
> We'll want to reflect changes to common files back to nbdkit; but it
> doesn't hold up the libnbd review.
> 
>> +++ b/common/include/test-array-size.c
>> @@ -41,21 +41,21 @@
>>  
>>  struct st { const char *s; int i; };
>>  
>> -static const char *s0[] __attribute__((__unused__)) = { };
>> -static const char *s1[] __attribute__((__unused__)) = { "a"
};
>> -static const char *s3[] __attribute__((__unused__)) = { "a",
"b", "c" };
>> -static const char *s4[4] __attribute__((__unused__)) = {
"a", "b", "c", "d" };
>> -static int i0[] __attribute__((__unused__)) = { };
>> -static int i1[] __attribute__((__unused__)) = { 1 };
>> -static int i3[] __attribute__((__unused__)) = { 1, 2, 3 };
>> -static int i4[4] __attribute__((__unused__)) = { 1, 2, 3, 4 };
>> -static struct st st0[] __attribute__((__unused__)) = { };
>> -static struct st st1[] __attribute__((__unused__)) = { {
"a", 1 } };
>> -static struct st st3[] __attribute__((__unused__)) >> +static
const char *s0[] __attribute__ ((__unused__)) = { };
>> +static const char *s1[] __attribute__ ((__unused__)) = { "a"
};
>> +static const char *s3[] __attribute__ ((__unused__)) = {
"a", "b", "c" };
>> +static const char *s4[4] __attribute__ ((__unused__)) = {
"a", "b", "c", "d" };
> 
> This one is still barely inside 80 columns, but did grab my eye as
> getting longer.
> 
>> +++ b/ocaml/nbd-c.h
>> @@ -32,7 +32,7 @@
>>  
>>  /* Workaround for OCaml < 4.06.0 */
>>  #ifndef Bytes_val
>> -#define Bytes_val(x) String_val(x)
>> +#define Bytes_val(x) String_val (x)
>>  #endif
>>  
>>  /* Wrapper around caml_alloc_custom_mem for pre-2019 versions of
OCaml. */
>> @@ -68,7 +68,7 @@ extern void nbd_internal_unix_sockaddr_to_sa (value,
struct sockaddr_storage *,
>>  extern void nbd_internal_ocaml_exception_in_wrapper (const char *,
value);
>>  
>>  /* Extract an NBD handle from an OCaml heap value. */
>> -#define NBD_val(v) (*((struct nbd_handle **)Data_custom_val(v)))
>> +#define NBD_val(v) (*((struct nbd_handle **)Data_custom_val (v)))
> 
> Hmm. Another potential cleanup patch (NOT for this one) would be
> settling on a preferred style for whether the cast prefix operator has
> a following space.  For example, in copy/file-ops.c, we use both
> styles (git grep ' \*) \?[a-zA-Z]') when casting to a single
pointer
> type (I didn't check double pointers or casts to a type name):
> 
> copy/file-ops.c:  struct rw_file *rwf = (struct rw_file *)rw;
> copy/file-ops.c:  struct rw_file *rwf = (struct rw_file *) rw;
> copy/file-ops.c:  struct rw_file *rwf = (struct rw_file *)rw;
> copy/file-ops.c:  struct rw_file *rwf = (struct rw_file *)rw;
> copy/file-ops.c:    data = (char *) data + r;
> copy/file-ops.c:  struct rw_file *rwf = (struct rw_file *)rw;
> copy/file-ops.c:    data = (char *) data + r;
Yes, good observation.
> 
> I think I'm a fan of having the space after the cast operator, and as
> long as we're doing tree-wide cleanups, this would be a good time to
> inject such a patch if we wanted.
I actually dislike the space there; the reason being that the cast
operator has one of the strongest bindings in C, and the space evokes
the opposite impression. We've had actual bugs introduced in edk2
because someone was misled by this.

(The cast prefix op is in the same group as "*", "&",
"!", "~", and
unary "-"; we don't use a space with those either.

... Well, I've seen a space character after "!" in some spots, and
I
happen to strongly disagree with that, for the exact same reason -- but
that's another discussion. :))
> 
> Also, it might be cool to have a code formatting tool automatically
> check that patches conform to a given style (but that presupposes that
> there is a tool out there that gives us a style we like, or where the
> differences to our current style are minimal to instead go with one of
> its builtin styles - AND that such a tool can be present on at least
> one of the CI machines to avoid regressions).  But that's a much
> bigger task, and I'm not up to doing it myself at the moment.
It's a monumental task. In edk2, just searching for the right had taken
very long, and once they settled on uncrustify, several patches were
required for upstream uncrustify, *plus* a humongous config file in edk2.
> Overall, I don't have any strong reasons to insist on shorter #ifdef
> spellings, and the rest of your changes, while mechanical, do make the
> codebase seem more consistent.  Tweaking the long line is minor enough
> that it could be done later if at all.
I certainly don't want to dismiss these observations, but I'll
definitely forget about them unless we record them somewhere. Do these
deserve a bugzilla (or multiple bugzillas)?
> Reviewed-by: Eric Blake <eblake at redhat.com>
Thanks!
Laszlo

On Mon, Feb 20, 2023 at 02:38:25PM +0100, Laszlo Ersek
wrote:
> > 
> > [1] This change widened out beyond 80 columns.  Is it worth splitting
> > that typedef line in two, perhaps as:
> > 
> >     typedef char NBDKIT_UNIQUE_NAME (_x_has_uint_type)               \
> >         [(typeof (x))-1 > 0 ? 1 : -1] __attribute__ ((__unused__));
\
> > 
> > or does that make it look worse?  At any rate, even if we do want to
> > reflow the line to be shorter, you have to consider the commit message
> > blurb about 'git show -w'.
> 
> This patch concerns itself with one thing only: the space character
> between the function designator (or macro name) and the paren that opens
> the argument list. Everything else is out of scope for the patch.
Fully agreed.  Doing JUST spacing is the only thing appropriate for this patch.
> 
> In the particular case, the original NBDKIT_UNIQUE_NAME line was already
> 84 characters long; in fact that original line, when introduced, broke
> the alignment of the backslashes at the right hand side. That state
> stems from nbdkit (not libnbd) commit cf2b6297646a
> ("common/include/unique-name.h: Rename UNIQUE_NAME macro",
2022-01-11).
> It was later ported to libnbd in commit 485f5ddf2d48
> ("common/include/unique-name.h: Rename UNIQUE_NAME macro",
2022-02-23).
> 
> So, this patch highlights another pre-existent task, and creates a new one:
> 
> - the overlong lines from the above-noted commits should be cleaned up
> 
> - the whitespace updates from the present patch should be reflected back
> to nbdkit.
> 
> I was aware of both of those tasks, it's just that my cleanup stack
> simply couldn't accommodate further recursions. I was already cleaning
> up a thing for a cleanup that I needed for another cleanup. I couldn't
> nest them any deeper, I had to stop the scope creep somewhere.
Fair enough. We do indeed seem good at adding to our TODO list without
meaning to.  We don't need to hold up this series waiting for other
cleanup tasks to be done.
> 
> More generally, we should probably invent a way to avoid porting such
> utility code back and forth between libnbd and nbdkit. For example,
> libguestfs, guestfs-tools, and virt-v2v share the "common"
submodule.
The idea makes sense to me, although it does add a bit of a pain (git
submodules are not always the easiest to work with).
> > 
> > Hmm. Another potential cleanup patch (NOT for this one) would be
> > settling on a preferred style for whether the cast prefix operator has
> > a following space.  For example, in copy/file-ops.c, we use both
> > styles (git grep ' \*) \?[a-zA-Z]') when casting to a single
pointer
> > type (I didn't check double pointers or casts to a type name):
> > 
> > copy/file-ops.c:  struct rw_file *rwf = (struct rw_file *)rw;
> > copy/file-ops.c:  struct rw_file *rwf = (struct rw_file *) rw;
> > copy/file-ops.c:  struct rw_file *rwf = (struct rw_file *)rw;
> > copy/file-ops.c:  struct rw_file *rwf = (struct rw_file *)rw;
> > copy/file-ops.c:    data = (char *) data + r;
> > copy/file-ops.c:  struct rw_file *rwf = (struct rw_file *)rw;
> > copy/file-ops.c:    data = (char *) data + r;
> 
> Yes, good observation.
> 
> > 
> > I think I'm a fan of having the space after the cast operator, and
as
> > long as we're doing tree-wide cleanups, this would be a good time
to
> > inject such a patch if we wanted.
> 
> I actually dislike the space there; the reason being that the cast
> operator has one of the strongest bindings in C, and the space evokes
> the opposite impression. We've had actual bugs introduced in edk2
> because someone was misled by this.
> 
> (The cast prefix op is in the same group as "*",
"&", "!", "~", and
> unary "-"; we don't use a space with those either.
> 
> ... Well, I've seen a space character after "!" in some
spots, and I
> happen to strongly disagree with that, for the exact same reason -- but
> that's another discussion. :))
You actually make a strong argument for not having the space after a
cast.  Thinking about it more, that also means that visually, you can
distinguish between
 (foo) (bar, baz)
 (foo)(bar, baz)

where the former is a function call through a function pointer foo
with two arguments, while the latter is an (unusual) cast of the
result of the comma operator to type foo.  Not that I'd ever expect to
encounter code where this visual distinction makes the code easier to
read (and proof that parsing C is not trivial, because how to parse
depends on the compiler knowing a priori whether the name on the left
is a type name or a function pointer name).
> 
> > 
> > Also, it might be cool to have a code formatting tool automatically
> > check that patches conform to a given style (but that presupposes that
> > there is a tool out there that gives us a style we like, or where the
> > differences to our current style are minimal to instead go with one of
> > its builtin styles - AND that such a tool can be present on at least
> > one of the CI machines to avoid regressions).  But that's a much
> > bigger task, and I'm not up to doing it myself at the moment.
> 
> It's a monumental task. In edk2, just searching for the right had taken
> very long, and once they settled on uncrustify, several patches were
> required for upstream uncrustify, *plus* a humongous config file in edk2.
edk2 has the drawback of a widely disparate group of contributors each
with their own preferred style and with a large existing corpus of
code.  At least with libnbd and nbdkit, we have a small enough group
of regular contributors and a smaller code base, where adopting an
existing style would require a one-time painful conversion (which in
turn would be awkward across git blame), but where getting consensus
on a style that could be automatically be enforced need not carry
baggage of a large config file.  But yeah, definitely not a priority
for today.
> 
> > Overall, I don't have any strong reasons to insist on shorter
#ifdef
> > spellings, and the rest of your changes, while mechanical, do make the
> > codebase seem more consistent.  Tweaking the long line is minor enough
> > that it could be done later if at all.
> 
> I certainly don't want to dismiss these observations, but I'll
> definitely forget about them unless we record them somewhere. Do these
> deserve a bugzilla (or multiple bugzillas)?
Bugzillas will help us remember the ideas.  The question is whether
they are worth sinking enough time into, or whether they just add to a
backlog that never rises to top priority.
> 
> > Reviewed-by: Eric Blake <eblake at redhat.com>
> 
> Thanks!
> Laszlo
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Mon, Feb 20, 2023 at 02:38:25PM +0100, Laszlo Ersek
wrote:
> I certainly don't want to dismiss these observations, but I'll
> definitely forget about them unless we record them somewhere. Do these
> deserve a bugzilla (or multiple bugzillas)?
You could drop a note in the TODO file.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org