Re: [PATCH 0/4] Unify getpkt methods in remote.c

[John Baldwin <jhb@FreeBSD.org>]

On 8/28/23 10:14 AM, Tom Tromey via Gdb-patches wrote:
> This series unifies the various getpkt methods in remote.c, and
> applies a little bit of bool-ification as well.
> 
> ---
> Tom Tromey (4):
>        Remove getpkt_sane
>        Remove expecting_notif parameter from getpkt_or_notif_sane_1
>        Use bool in getpkt
>        Unify getpkt and getpkt_or_notif_sane
> 
>   gdb/remote.c | 242 +++++++++++++++++++++++++----------------------------------
>   1 file changed, 103 insertions(+), 139 deletions(-)
> ---
> base-commit: b8a175b415454df6a039ba0b5d2ff13c3c180275
> change-id: 20230828-getpkt-cleanup-126c625821bf
> 
> Best regards,

These all look fine to me.  My only thought was if you wanted to
make use of a default value for the 'forever' argument, or perhaps
have a 'getpkt_wait' wrapper.  My one worry about bool arguments
to functions (in general) is that true/false often do not convey
obvious meaning in context.  One way to deal with this can be to
use dedicated enums, e.g.:

enum getpkt_forever {
     NON_BLOCKING,
     BLOCKING
}

so that the invocations have more obvious intent.  However, reading
the patch 3 in particular, it seems like the vast majority of getpkt
calls end up passing false, so I wonder if you might not like to
end up with something like this at the end:

    int getpkt_1 (gdb::char_vector *buf, bool forever, bool *is_notif);
    int getpkt (gdb::char_vector *buf, bool *is_notif = nullptr)
    { return getpkt_1 (buf, false, is_notif); }
    int getpkt_wait (gdb::char_vector *buf, bool *is_notif = nullptr)
    { return getpkt_1 (buf, true, is_notif); }

Maybe you could get by with defaulting both arguments to getpkt and
not needing getpkt_1 at all, just having getpkt_wait() as wrapper?
I think that might end up being more readable as in many cases in
the resulting code you'd end up with:

    putpkt ("<mumble>");
    getpkt (&rs->buf);

(You could also call the wrapper `getpkt_forever` if that seems like
a better name.)

-- 
John Baldwin