Libguestfs - Apr 2023 - [libnbd PATCH 00/18] wrap hand-written source code at 80 characters

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/5d94be2583d557cfc7f8a8cfee7988abfa45a3f8/daemon/daemon-c.c#L40
>    */
> 
> 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.

> 
> 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.

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 at redhat.com>

whether or not you touch things up.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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/5d94be2583d557cfc7f8a8cfee7988abfa45a3f8/daemon/daemon-c.c#L40
>>    */
>>
>> 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 at 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