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

Simon Marchi
2017-01-17T19:57:16Z

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