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

[Simon Marchi <simon.marchi@polymtl.ca>]

I've only looked at ui-file.h/c so far, but overall it looks really 
nice.

On 2017-01-16 20:39, Pedro Alves wrote:
> -static void
> -null_file_fputs (const char *buf, struct ui_file *file)
> -{
> -  if (file->to_write == null_file_write)
> -    /* Both the write and fputs methods are null.  Discard the
> -       request.  */
> -    return;
> -  else
> -    {
> -      /* The write method was implemented, use that.  */
> -      file->to_write (file, buf, strlen (buf));
> -    }
> -}
> +\f

Is the insertion of a form feed character on purpose?  I know it's 
mentioned in the GNU coding style, but I was wondering if they were 
useful to anybody today.

> diff --git a/gdb/ui-file.h b/gdb/ui-file.h
> index 4ad3940..f1951e4 100644
> --- a/gdb/ui-file.h
> +++ b/gdb/ui-file.h
> @@ -24,74 +24,86 @@ struct ui_file;

The forward declarations at the beginning of the file can go.

> 
>  #include <string>
> 
> -/* Create a generic ui_file object with null methods.  */
> -
> -extern struct ui_file *ui_file_new (void);
> -
> -/* Override methods used by specific implementations of a UI_FILE
> -   object.  */
> -
> -typedef void (ui_file_flush_ftype) (struct ui_file *stream);
> -extern void set_ui_file_flush (struct ui_file *stream,
> -			       ui_file_flush_ftype *flush);
> -
> -/* NOTE: Both fputs and write methods are available.  Default
> -   implementations that mapping one onto the other are included.  */
> -typedef void (ui_file_write_ftype) (struct ui_file *stream,
> -				    const char *buf, long length_buf);
> -extern void set_ui_file_write (struct ui_file *stream,
> -			       ui_file_write_ftype *fputs);
> -
> -typedef void (ui_file_fputs_ftype) (const char *, struct ui_file 
> *stream);
> -extern void set_ui_file_fputs (struct ui_file *stream,
> -			       ui_file_fputs_ftype *fputs);
> -
> -/* This version of "write" is safe for use in signal handlers.
> -   It's not guaranteed that all existing output will have been
> -   flushed first.
> -   Implementations are also free to ignore some or all of the request.
> -   fputs_async is not provided as the async versions are rarely used,
> -   no point in having both for a rarely used interface.  */
> -typedef void (ui_file_write_async_safe_ftype)
> -  (struct ui_file *stream, const char *buf, long length_buf);
> -extern void set_ui_file_write_async_safe
> -  (struct ui_file *stream, ui_file_write_async_safe_ftype 
> *write_async_safe);
> -
> -typedef long (ui_file_read_ftype) (struct ui_file *stream,
> -				   char *buf, long length_buf);
> -extern void set_ui_file_read (struct ui_file *stream,
> -			      ui_file_read_ftype *fread);
> -
> -typedef int (ui_file_isatty_ftype) (struct ui_file *stream);
> -extern void set_ui_file_isatty (struct ui_file *stream,
> -				ui_file_isatty_ftype *isatty);
> -
> -typedef void (ui_file_rewind_ftype) (struct ui_file *stream);
> -extern void set_ui_file_rewind (struct ui_file *stream,
> -				ui_file_rewind_ftype *rewind);
> -
>  typedef void (ui_file_put_method_ftype) (void *object, const char 
> *buffer,
>  					 long length_buffer);

This one can go too (actually you've marked it as deleted in the 
changelog).

> -typedef void (ui_file_put_ftype) (struct ui_file *stream,
> -				  ui_file_put_method_ftype *method,
> -				  void *context);
> -extern void set_ui_file_put (struct ui_file *stream, ui_file_put_ftype 
> *put);
> 
> -typedef void (ui_file_delete_ftype) (struct ui_file * stream);
> -extern void set_ui_file_data (struct ui_file *stream, void *data,
> -			      ui_file_delete_ftype *to_delete);
> +/* The ui_file base class.  */
> 
> -typedef int (ui_file_fseek_ftype) (struct ui_file *stream, long 
> offset,
> -				   int whence);
> -extern void set_ui_file_fseek (struct ui_file *stream,
> -			       ui_file_fseek_ftype *fseek_ptr);
> +class ui_file
> +{
> +public:
> +  ui_file ();
> +  virtual ~ui_file ();
> 
> -extern void *ui_file_data (struct ui_file *file);
> +  /* Public non-virtual API.  */
> 
> +  void printf (const char *, ...) ATTRIBUTE_PRINTF (2, 3);
> 
> -extern void gdb_flush (struct ui_file *);
> +  /* Print a string whose delimiter is QUOTER.  Note that these
> +     routines should only be call for printing things which are

call -> called?

> +  /* Rewind local buffer.  I.e., clear it.  */
> +  virtual void rewind ()
> +  {}

I'm not sure about the implications, but instinctively I would've made 
rewind in the base class throw an exception by default, instead of 
making the derived types throw one if they don't support rewind.  
Actually, except for the null_file, I don't really see when you would 
want rewind to be a no-op.  If the rewind call returns but does not 
actually rewind, isn't it lying to the client code?

> 
> -extern void ui_file_delete (struct ui_file *stream);
> +  /* Write contents of local buffer onto WRITE.  */
> +  virtual void write_buffer_on (ui_file &where)

Mismatch between parameter name and doc.

> +  /* Provide a few convenience methods with the same API as the
> +     underlying std::string.  */
> +  const char *data () const { return m_string.data (); }
> +  const char *c_str () const { return m_string.c_str (); }
> +  size_t size () const { return m_string.size (); }
> +  bool empty () const;

I am wondering why you didn't implement empty inline like the others, 
since it's very similar.

> +
> +private:
> +  /* The internal buffer.  */
> +  std::string m_string;
> +};
> +
> +/* A ui_file implementation that maps directly onto <stdio.h>'s
> +   FILE.  */
> +
> +class stdio_file : public ui_file
> +{
> +public:
> +  /* Create a ui_file from a previously opened FILE.  If not CLOSE_P,
> +     then behave like like fdopen().  */

"like like".  It's also not very clear just saying "behave like 
fdopen()".  Could it be a bit more explicit?

> +  explicit stdio_file (FILE *file, bool close_p = false);
> +
> +  /* Create an stdio_file that is not managing any file yet.  Call
> +     open to actually open something.  */
> +  stdio_file ();
> +
> +  ~stdio_file () override;
> +
> +  /* Open NAME in mode MODE.  Returns true on success, false
> +     otherwise.  Destroying the stdio_file closes the underlying FILE
> +     handle.  */
> +  bool open (const char *name, const char *mode);

What happens if you try to reuse that object by calling open twice?  
Does it leak an open file?

> +class tee_file : public ui_file
> +{
> +public:
> +  /* Create a file which writes to both ONE and TWO.  CLOSE_ONE and
> +     CLOSE_TWO indicate whether the original files should be closed
> +     when the new file is closed.  */
> +  tee_file (ui_file *one, bool close_one,
> +	    ui_file *two, bool close_two);
> +  ~tee_file () override;
> 
> -/* Open/create a STDIO based UI_FILE using the already open FILE.  */
> -extern struct ui_file *stdio_fileopen (FILE *file);
> +  void write (const char *buf, long length_buf) override;
> +  void write_async_safe (const char *buf, long length_buf) override;
> +  void puts (const char *) override;
> 
> -/* Likewise, for stderr-like streams.  */
> -extern struct ui_file *stderr_fileopen (FILE *file);
> +  bool isatty () override;
> +  void flush () override;
> 
> +  void rewind () override
> +  { gdb_assert_not_reached ("tee_file::rewind called\n"); }
> 
> -/* Open NAME returning an STDIO based UI_FILE.  */
> -extern struct ui_file *gdb_fopen (const char *name, const char *mode);
> +private:
> +  /* The two underlying ui_files, and whether they should each be
> +     closed on destruction.  */
> +  struct ui_file *m_one, *m_two;

Nit: I'm a big fan of dropping the struct keyword.

Thanks,

Simon