[PATCH v2 5/5] Eliminate make_cleanup_ui_file_delete / make ui_file a class hierarchy

[Simon Marchi]

On 2017-01-23 18:18, Pedro Alves wrote:
> @@ -195,20 +188,18 @@ handle_redirections (int from_tty)
>        return;
>      }
> 
> -  output = gdb_fopen (logging_filename, logging_overwrite ? "w" : 
> "a");
> -  if (output == NULL)
> +  std::unique_ptr<stdio_file> log (new stdio_file ());

What is the "rule" for using std::unique_ptr directly vs defining a 
typedef?

   typedef std::unique_ptr<stdio_file> stdio_file_up;

I'd like if we could avoid having a mix of styles, unless there are good 
reasons for it.

> +  if (!log->open (logging_filename, logging_overwrite ? "w" : "a"))
>      perror_with_name (_("set logging"));
> -  cleanups = make_cleanup_ui_file_delete (output);
> +
> +  output = log.get ();
> 
>    /* Redirects everything to gdb_stdout while this is running.  */
>    if (!logging_redirect)
>      {
>        no_redirect_file = output;
> 
> -      output = tee_file_new (gdb_stdout, 0, no_redirect_file, 0);
> -      if (output == NULL)
> -	perror_with_name (_("set logging"));
> -      make_cleanup_ui_file_delete (output);
> +      output = new tee_file (gdb_stdout, 0, no_redirect_file, 0);

Is there anything that replaces this cleanup?  IOW, if the 
fprinf_unfiltered just below did throw an exception for some reason, 
would this tee_file leak?

>        if (from_tty)
>  	fprintf_unfiltered (gdb_stdout, "Copying output to %s.\n",
>  			    logging_filename);

...

> @@ -420,22 +412,20 @@ c_compute_program (struct compile_instance *inst,
>  			   ? "&" : ""));
>        break;
>      default:
> -      fputs_unfiltered (input, buf);
> +      buf.puts (input);
>        break;
>      }
> 
> -  fputs_unfiltered ("\n", buf);
> +  buf.puts ("\n");
> 
>    /* For larger user expressions the automatic semicolons may be
>       confusing.  */
>    if (strchr (input, '\n') == NULL)
> -    fputs_unfiltered (";\n", buf);
> +    buf.puts (";\n");
> 
>    if (inst->scope != COMPILE_I_RAW_SCOPE)
> -    fputs_unfiltered ("}\n", buf);
> +    buf.puts ("}\n");
> 
> -  add_code_footer (inst->scope, buf);
> -  code = ui_file_as_string (buf);
> -  do_cleanups (cleanup);
> -  return code;
> +  add_code_footer (inst->scope, &buf);
> +  return std::move (buf.string ());

I would have thought that this std::move would be superfluous, because 
the compiler would do it anyway.  Is it the case?  Is it a good practice 
to use move explicitly to make sure it's a move and not a copy (and 
probably get a compile-time error if a move is not possible)?

> diff --git a/gdb/guile/scm-disasm.c b/gdb/guile/scm-disasm.c
> index d06c481..bf3c424 100644
> --- a/gdb/guile/scm-disasm.c
> +++ b/gdb/guile/scm-disasm.c
> @@ -162,7 +162,7 @@ gdbscm_disasm_print_address (bfd_vma addr, struct
> disassemble_info *info)
>  static int
>  gdbscm_print_insn_from_port (struct gdbarch *gdbarch,
>  			     SCM port, ULONGEST offset, CORE_ADDR memaddr,
> -			     struct ui_file *stream, int *branch_delay_insns)
> +			     string_file *stream, int *branch_delay_insns)

Reference instead of pointer?  I don't really mind, it's just that 
you've used references in other places (e.g. the compile code) when 
changing a ui_file pointer to a more specific type.

> diff --git a/gdb/guile/scm-ports.c b/gdb/guile/scm-ports.c
> index 4a1c864..fb3a47b 100644
> --- a/gdb/guile/scm-ports.c
> +++ b/gdb/guile/scm-ports.c
> @@ -37,11 +37,18 @@
> 
>  /* A ui-file for sending output to Guile.  */
> 
> -typedef struct
> +class ioscm_file_port : public ui_file
>  {
> -  int *magic;
> -  SCM port;
> -} ioscm_file_port;
> +public:
> +  /* Return a ui_file that writes to PORT.  */
> +  explicit ioscm_file_port (SCM port);
> +
> +  void flush () override;
> +  void write (const char *buf, long length_buf) override;
> +
> +private:
> +  SCM m_port;
> +};
> 
>  /* Data for a memory port.  */
> 
> @@ -431,74 +438,21 @@ gdbscm_error_port (void)
>  
>  /* Support for sending GDB I/O to Guile ports.  */
> 
> -static void
> -ioscm_file_port_delete (struct ui_file *file)
> -{
> -  ioscm_file_port *stream = (ioscm_file_port *) ui_file_data (file);
> -
> -  if (stream->magic != &file_port_magic)
> -    internal_error (__FILE__, __LINE__,
> -		    _("ioscm_file_port_delete: bad magic number"));
> -  xfree (stream);
> -}
> -
> -static void
> -ioscm_file_port_rewind (struct ui_file *file)
> -{
> -  ioscm_file_port *stream = (ioscm_file_port *) ui_file_data (file);
> -
> -  if (stream->magic != &file_port_magic)
> -    internal_error (__FILE__, __LINE__,
> -		    _("ioscm_file_port_rewind: bad magic number"));
> -
> -  scm_truncate_file (stream->port, 0);
> -}
> +ioscm_file_port::ioscm_file_port (SCM port)
> +  : m_port (port)
> +{}
> 
> -static void
> -ioscm_file_port_put (struct ui_file *file,
> -		     ui_file_put_method_ftype *write,
> -		     void *dest)
> +void
> +ioscm_file_port::flush ()
>  {
> -  ioscm_file_port *stream = (ioscm_file_port *) ui_file_data (file);
> -
> -  if (stream->magic != &file_port_magic)
> -    internal_error (__FILE__, __LINE__,
> -		    _("ioscm_file_port_put: bad magic number"));
> -
> -  /* This function doesn't meld with ports very well.  */
>  }

It looks like there's a low-level function, scm_force_output, that 
"flushes" a port.  I wonder if we'd want to do that in flush.

> diff --git a/gdb/mi/mi-console.c b/gdb/mi/mi-console.c
> index afb5e94..218ffbc 100644
> --- a/gdb/mi/mi-console.c
> +++ b/gdb/mi/mi-console.c
> @@ -26,118 +26,62 @@
> 
>  #include "defs.h"
>  #include "mi-console.h"
> -static ui_file_fputs_ftype mi_console_file_fputs;
> -static ui_file_flush_ftype mi_console_file_flush;
> -static ui_file_delete_ftype mi_console_file_delete;
> -
> -struct mi_console_file
> -  {
> -    int *magic;
> -    struct ui_file *raw;
> -    struct ui_file *buffer;
> -    const char *prefix;
> -    char quote;
> -  };
> -
> -/* Use the address of this otherwise-unused global as a magic number
> -   identifying this class of ui_file objects.  */
> -static int mi_console_file_magic;
> 
>  /* Create a console that wraps the given output stream RAW with the
>     string PREFIX and quoting it with QUOTE.  */
> 
> -struct ui_file *
> -mi_console_file_new (struct ui_file *raw, const char *prefix, char 
> quote)
> -{
> -  struct ui_file *ui_file = ui_file_new ();
> -  struct mi_console_file *mi_console = XNEW (struct mi_console_file);
> -
> -  mi_console->magic = &mi_console_file_magic;
> -  mi_console->raw = raw;
> -  mi_console->buffer = mem_fileopen ();
> -  mi_console->prefix = prefix;
> -  mi_console->quote = quote;
> -  set_ui_file_fputs (ui_file, mi_console_file_fputs);
> -  set_ui_file_flush (ui_file, mi_console_file_flush);
> -  set_ui_file_data (ui_file, mi_console, mi_console_file_delete);
> -
> -  return ui_file;
> -}
> +mi_console_file::mi_console_file (ui_file *raw, const char *prefix, 
> char quote)
> +  : m_raw (raw),
> +    m_prefix (prefix),
> +    m_quote (quote)
> +{}
> 
> -static void
> -mi_console_file_delete (struct ui_file *file)
> +void
> +mi_console_file::puts (const char *buf)
>  {
> -  struct mi_console_file *mi_console
> -    = (struct mi_console_file *) ui_file_data (file);
> -
> -  if (mi_console->magic != &mi_console_file_magic)
> -    internal_error (__FILE__, __LINE__,
> -		    _("mi_console_file_delete: bad magic number"));
> -
> -  xfree (mi_console);
> +  this->write (buf, strlen (buf));
>  }

mi_console_file::puts looks identical to ui_file::puts.  Is there a 
reason to overload it?

> @@ -1516,15 +1509,13 @@ mi_cmd_data_read_memory (char *command, char
> **argv, int argc)
> 
>    /* Build the result as a two dimentional table.  */
>    {
> -    struct ui_file *stream;
> -    struct cleanup *cleanup_stream;
>      int row;
>      int row_byte;
> +    struct cleanup *cleanup_list;
> 
> -    stream = mem_fileopen ();
> -    cleanup_stream = make_cleanup_ui_file_delete (stream);
> +    string_file stream;
> 
> -    make_cleanup_ui_out_list_begin_end (uiout, "memory");
> +    cleanup_list = make_cleanup_ui_out_list_begin_end (uiout, 
> "memory");
>      for (row = 0, row_byte = 0;
>  	 row < nr_rows;
>  	 row++, row_byte += nr_cols * word_size)

Throughout mi_cmd_data_read_memory, is is worth it performance-wise to 
re-use the same string_file and doing some .clear() rather than 
declaring different ones in more specific scopes?  Since they are stack 
allocated, there shouldn't be any runtime cost in execution time.  Which 
is faster between constructing a string_file and calling .clear()?

I think it would be less error-prone (like forgetting to call .clear() 
between two usages) to declare multiple string_file objects in the 
scopes that need them, but I understand if you'd rather not do it for 
performance reasons.  I also understand if you say that it's because 
it's the way it worked and don't want to spend time on a deprecated 
function anyway :).

>    /* Expand tabs into spaces, since ncurses on MS-Windows doesn't.  */
> -  ret = tui_expand_tabs (p, 0);
> +  ret = tui_expand_tabs (str.c_str (), 0);

It's not in the scope of this patch, but it would be nice to rewrite 
tui_expand_tabs using C++ features, I'm sure it could be much simpler.


I'm not done looking at the patch yet, but I've deleted to much of the 
quote and I have comments about stuff I deleted, so it will be simpler 
to just send another e-mail.