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

[Simon Marchi <simark@simark.ca>]

On 2020-08-12 5:39 p.m., Aaron Merey wrote:
> Hi Simon,
> 
> Thanks for the review, I've updated the patch and moved the Makefile.in
> change to a separate patch.
> 
> Aaron
> 

Hi Aaron,

Thanks, just a few minor things, the patch LGTM with those fixed.

> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index f4a227b040..3f51aaaf43 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -43,29 +43,37 @@ 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
> +{
> +  user_data (const char *desc, const char *fname, bool has_printed)
> +    : desc (desc), fname (fname), has_printed (has_printed)

There's no need for the `has_printed` parameter.  Just initialize the `has_printed` field to false below.

> +  { }
> +
> +  const char * const desc;
> +  const char * const fname;
> +  bool has_printed;
> +};
>
>  static int
>  progressfn (debuginfod_client *c, long cur, long total)
>  {
> +  user_data *data = static_cast<user_data *> (debuginfod_get_user_data (c));
> +
>    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,
> +		       styled_string (file_name_style.style (), data->fname));
>        return 1;
>      }
>
> -  if (!has_printed && total != 0)
> +  if (!data->has_printed && total != 0)
>      {
>        /* Print this message only once.  */
> -      has_printed = true;
> +      data->has_printed = true;
>        printf_filtered ("Downloading %s %ps...\n",
> -		       desc.c_str (),
> -		       styled_string (file_name_style.style (), fname.c_str ()));
> +		       data->desc,
> +		       styled_string (file_name_style.style (), data->fname));
>      }
>
>    return 0;
> @@ -98,10 +106,9 @@ debuginfod_source_query (const unsigned char *build_id,
>    if (c == nullptr)
>      return scoped_fd (-ENOMEM);
>
> -  desc = std::string ("source file");
> -  fname = std::string (srcpath);
> -  has_printed = false;
> +  struct user_data data ("source file", srcpath, false);

Omit the `struct` keyword when referring to the new type.

>
> +  debuginfod_set_user_data (c, &data);
>    scoped_fd fd (debuginfod_find_source (c,
>  					build_id,
>  					build_id_len,
> @@ -136,11 +143,10 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
>    if (c == nullptr)
>      return scoped_fd (-ENOMEM);
>
> -  desc = std::string ("separate debug info for");
> -  fname = std::string (filename);
> -  has_printed = false;
>    char *dname = nullptr;
> +  struct user_data data ("separate debug info for", filename, false);

Same here.

Simon