On 2/20/23 18:41, Eric Blake wrote:
> 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.
I didn't notice we had a TODO file! :)
>
>>
>> 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.
OK, when I sit down to go through the complete set of review comments
for this series, I'll capture the "for later" items in a bugzilla ticket
(or several tickets). I don't mind working on these items, and I've had
luck with bugzilla in general, for tracking stuff.
Thanks!
Laszlo
>
>>
>>> Reviewed-by: Eric Blake <ebl...@redhat.com>
>>
>> Thanks!
>> Laszlo
>>
>
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
