[Libguestfs] [libnbd PATCH v3 01/29] use space consistently in function and function-like macro invocations
Laszlo Ersek
lersek at redhat.com
Mon Feb 20 13:38:25 UTC 2023
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
More information about the Libguestfs
mailing list