11:41 a.m.
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(a)redhat.com>
Thanks!
Laszlo
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org