Re: [PATCH] debuginfod-support.c: Replace globals with user_data

Simon Marchi
2020-08-01T14:45:04Z

> From 1913317cb95d02c3244e5d91b7b59e33cfe991c5 Mon Sep 17 00:00:00 2001
> From: Aaron Merey <amerey@redhat.com>
> Date: Fri, 31 Jul 2020 18:25:48 -0400
> Subject: [PATCH] debuginfod-support.c: Replace globals with user_data
>
> Store query information in user_data struct instead of global
> variables. Also include DEBUGINFOD_CFLAGS in INTERNAL_CFLAG_BASE.

The DEBUGINFOD_CFLAGS change looks good, but it's unrelated to the topic of this patch,
please make it its own patch, with its own justification.

> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index f4a227b040..9325bf8651 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -43,29 +43,39 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
> #else
> #include <elfutils/debuginfod.h>
>
> -/* TODO: Use debuginfod API extensions instead of these globals. */
> -static std::string desc;
> -static std::string fname;
> -static bool has_printed;
> +struct user_data
> +{
> + std::string desc;
> + std::string fname;
> + bool has_printed;
> +
> + user_data (std::string desc, std::string fname, bool has_printed)
> + : desc (desc), fname (fname), has_printed (has_printed)
> + { }

Put the constructor before the fields. Make the fields const if they are not intended
to be changed once the object has been created.

Using `std::string` causes unnecessary copying to happen. Just use `const char *`
(`const char * const` for the fields) or gdb::string_view, if you want to be more
generic (but all callers are `const char *`, so it's not really necessary). That's
assuming that the lifetime of those strings will always be longer than the lifetime
of the debuginfo client. I think that's the case, since the debug info client is
always created and destroyed in the same function.

> +};
>
> static int
> progressfn (debuginfod_client *c, long cur, long total)
> {
> + struct user_data *data = static_cast<user_data*> (debuginfod_get_user_data (c));

This line is too long, just skip the `struct` keyword and you'll be fine. Also, space
before `*`.

> +
> if (check_quit_flag ())
> {
> printf_filtered ("Cancelling download of %s %ps...\n",
> - desc.c_str (),
> - styled_string (file_name_style.style (), fname.c_str ()));
> + data->desc.c_str (),
> + styled_string (file_name_style.style (),
> + data->fname.c_str ()));
> return 1;
> }
>
> - if (!has_printed && total != 0)
> + if (! data->has_printed && total != 0)

Remove space after `!`.

Simon