Re: [PING*4][PATCH v4] gdb: Improve debuginfod progress updates

[Tom Tromey <tom@tromey.com>]

>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

Aaron> Ping
Aaron> Thanks,
Aaron> Aaron

Thanks for the patch.

>> +  cli_progress_info info;
>> +
>> +  info.pos = 0;
>> +  info.state = progress_update::START;

I think it would be better to have cli_progress_info initialize itself
instead of doing it here.

>> +  m_progress_info.push_back (std::move (info));

This could be just emplace_back then.

>> +  {
>> +    /* Position of the progress indicator.  */
>> +    int pos;

like this could be '= 0'

>> +    /* The current state.  */
>> +    progress_update::state state;

'= progress_update::START'

>> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
>> index 5f04a2b38ca..fc62412fff2 100644
>> --- a/gdb/debuginfod-support.c
>> +++ b/gdb/debuginfod-support.c
>> @@ -24,7 +24,9 @@
>> #include "gdbsupport/gdb_optional.h"
>> #include "cli/cli-cmds.h"
>> #include "cli/cli-style.h"
>> +#include "cli-out.h"
>> #include "target.h"
>> +#include <sstream>

Is this include used?

>> +/* Convert SIZE into a unit suitable for use with progress updates.
>> +   SIZE should in given in bytes and will be converted into KB, MB, GB
>> +   or remain unchanged. UNIT will be set to "B", "KB", "MB" or "GB"
>> +   accordingly.  */

There's a bunch of pedantic thought about the proper suffixes, but I
think we should just follow GNU coreutils and use the shorter B/K/M/G.

df --help says:

    The SIZE argument is an integer and optional unit (example: 10K is 10*1024).
    Units are K,M,G,T,P,E,Z,Y (powers of 1024) or KB,MB,... (powers of 1000).
    Binary prefixes can be used, too: KiB=K, MiB=M, and so on.

Also since all the responses are just a constant string, it's better to
just return a 'const char *' from the function and avoid allocation.

>> +/* Print the result of the most recent attempted download.  */
>> +
>> +static void
>> +print_outcome (user_data &data, int fd)
>> +{
>> +  /* Clears the current line of progress output.  */
>> +  current_uiout->do_progress_end ();
>> +
>> +  string_file styled_fname (current_uiout->can_emit_style_escape ());
>> +  fprintf_styled (&styled_fname, file_name_style.style (), "%s",
>> +                 data.fname);
>> +
>> +  if (fd < 0 && fd != -ENOENT)
>> +    gdb_printf (_("Download failed: %s. Continuing without %s %s.\n"),
>> +               safe_strerror (-fd), data.desc, styled_fname.c_str ());

I think you can use %ps and styled_string here instead of a separate
string_file.  Not sure this applied elsewhere but it's worth looking.

Hmm the old code did it:

>> -  if (fd.get () < 0 && fd.get () != -ENOENT)
>> -    gdb_printf (_("Download failed: %s.  Continuing without source file %ps.\n"),
>> -               safe_strerror (-fd.get ()),
>> -               styled_string (file_name_style.style (),  srcpath));

Seems like that can just be reused.

>> +void
>> +mi_ui_out::do_progress_start ()
>> +{
>> +  mi_progress_info info;
>> +
>> +  info.state = progress_update::START;

I don't understand why this creates an object and pushes it on a
vector.  Wouldn't printing the message from do_progress_start be the
same, but avoid all the hair?

>> +      struct ui_file *stream = gdb_stdout;
>> +      gdb_printf (stream, "%s...\n", msg.c_str ());

I don't think an explicit stream is needed here.

Nothing ever pops the mi_progress_info from the stack.

>> void
>> diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h
>> index 36d7e42345f..d80a7fce259 100644
>> --- a/gdb/mi/mi-out.h
>> +++ b/gdb/mi/mi-out.h
>> @@ -25,7 +25,6 @@
>> struct ui_out;
>> struct ui_file;
>> 
>> -
>> class mi_ui_out : public ui_out

Spurious change.

Tom