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 <ler...@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'.
> +++ 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;
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.
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.
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.
Reviewed-by: Eric Blake <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
