Re: [Libguestfs] [libnbd PATCH 00/18] wrap hand-written source code at 80 characters
On 4/19/23 15:40, Eric Blake wrote:
On Tue, Apr 18, 2023 at 07:26:13PM +0200, Laszlo Ersek wrote:
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2172516
[I started this email yesterday, then postponed it while looking at
individual patches...]
>
> This series wraps the non-generated C-language source code (*.c and *.h
> files) at 80 characters.
>
> "ocaml/helpers.c" remains overlong, but I couldn't find a way to wrap
> it: its single overlong line contains the comment
>
> /* For how we're getting the exception name, see:
> *
https://github.com/libguestfs/libguestfs/blob/5d94be2583d557cfc7f8a8cfee7...
> */
>
> and even if I truncate the blob hash to 12 nibbles, the line remains too
> long.
Truncating is fine to reduce the worst of the width, but I also
understand your reluctance to trim too short (git defaults to 7
nibbles in a fresh repository, but larger repositories like linux.git
output at least 10 nibbles and sometimes more because there are just
that many more hash prefix collisions as history grows - it's never
fun when a link valid today stops working tomorrow when a prefix
collision is introduced into the repo). At any rate, I have no
problems with long URLs in source files that are otherwise
length-constrained.
Right, I seem to remember Linux now recommends capturing 12 nibbles.
>
> The following files are also too wide:
>
> include/libnbd.h
> lib/api.c
> lib/states-run.c
> lib/states.c
> lib/states.h
> lib/unlocked.h
> ocaml/nbd-c.c
> python/libnbdmod.c
> python/methods.h
>
> but they are all generated; we'll have to discuss them separately.
Wrapping a generated file for legibility is definitely harder work;
legible generated code still has its benefits, but longer generated
lines for faster coding of the generator is a tolerable tradeoff in my
book.
So Rich pointed out in <https://bugzilla.redhat.com/show_bug.cgi?id=2172516#c3> that
the generator already had wrapping goodies.
The problem that made me put generated code aside immediately was not a lack of wrapping;
instead, the code was nicely wrapped (such that I couldn't improve it even by manually
tweaking the result!), but it was *still* too wide! Consider, from
"include/libnbd.h":
--------
extern int nbd_aio_opt_list_meta_context_queries (struct nbd_handle *h,
char **queries,
nbd_context_callback context_callback,
nbd_completion_callback
completion_callback)
LIBNBD_ATTRIBUTE_NONNULL (1, 2);
--------
That's 94 chars wide; even if we drop the "extern ", it remains 87
characters wide. :/
So I wanted to keep that discussion separate.
Overall, the series looked okay to me at a first read through; I did
spot some things on individual patches where I made comments, but they
are of the nature where I'm also okay with you adding:
Reviewed-by: Eric Blake <eblake(a)redhat.com>
whether or not you touch things up.
I'll go through the patches and see how I feel about just pushing them with the
suggested updates, or posting v2.
Thanks!
Laszlo