Re: [PATCH 19/22] Class-ify ui_out_impl

Pedro Alves
2016-11-30T13:09:28Z

On 11/26/2016 03:19 PM, simon.marchi@polymtl.ca wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
>
> This patch makes the ui_out_impl implementations an actual class
> hierarchy, instead of an array of function pointers.

LGTM with nits below.

> +ui_file *
> +cli_out_set_stream (struct ui_out *uiout, ui_file *stream)
> +{
> + cli_ui_out_impl *impl = dynamic_cast<cli_ui_out_impl *> (ui_out_impl (uiout));

"dynamic_cast" is a code smell. In this case, I think it
stems from the fact that we have separate ui_out and ui_out_impl
hierarchies. If we had a simple hierarchy, then this function would
take a cli_ui_out pointer as argument, or even be a
method of cli_ui_out.

Anyway, just a comment for another day.

Given the current design, this is OK.

> +
> + gdb_assert (impl != NULL);
> +
> + return impl->set_stream (stream);
> +}
> +
> /* CLI interface to display tab-completion matches. */
>
> /* CLI version of displayer.crlf. */
> diff --git a/gdb/cli-out.h b/gdb/cli-out.h
> index 296b8c0..aacb8c5 100644
> --- a/gdb/cli-out.h
> +++ b/gdb/cli-out.h
> @@ -23,22 +23,58 @@
> #include "ui-out.h"
> #include <vector>
>
> -/* These are exported so that they can be extended by other `ui_out'
> - implementations, like TUI's. */
> +class cli_ui_out_impl : public ui_out_impl_base
> +{
> +public:
>
> -struct cli_ui_out_data
> - {
> - std::vector<ui_file *> streams;
> - int suppress_output;
> - };
> + cli_ui_out_impl (ui_file *stream);

explicit.

> + virtual ~cli_ui_out_impl ();
>
> -extern const struct ui_out_impl cli_ui_out_impl;
> + virtual void table_begin (int nbrofcols, int nr_rows,
> + const char *tblid) override;
> + virtual void table_body () override;
> + virtual void table_end () override;
> + virtual void table_header (int width, ui_align align,
> + const std::string &col_name,
> + const std::string &col_hdr) override;
> + /* Note: level 0 is the top-level so LEVEL is always greater than
> + zero. */
> + virtual void begin (ui_out_type type, const char *id) override;
> + virtual void end (ui_out_type type) override;
> + virtual void field_int (int fldno, int width, ui_align align,
> + const char *fldname, int value) override;
> + virtual void field_skip (int fldno, int width, ui_align align,
> + const char *fldname) override;
> + virtual void field_string (int fldno, int width, ui_align align,
> + const char *fldname, const char *string) override;
> + virtual void ATTRIBUTE_PRINTF(6,0) field_fmt (int fldno, int width,
> + ui_align align,
> + const char *fldname,
> + const char *format,
> + va_list args) override;
> + virtual void spaces (int numspaces) override;
> + virtual void text (const char *string) override;
> + virtual void ATTRIBUTE_PRINTF(2,0)
> + message (const char *format, va_list args) override;
> + virtual void wrap_hint (const char *identstring) override;
> + virtual void flush () override;
> + virtual int redirect (struct ui_file * outstream) override;
>
> + ui_file *set_stream (ui_file *stream);
>
> -extern struct ui_out *cli_out_new (struct ui_file *stream);
> +protected:
> +
> + std::vector<ui_file *> m_streams;
> + int m_suppress_output;
> +
> +private:
> + void field_separator (void);

"(void)". Probably easier to grep the whole patch set to
find such cases and handle all of them.

> +mi_ui_out_impl::table_begin (int nr_cols, int nr_rows,
> + const char *tblid)
> {
> - mi_open (uiout, tblid, ui_out_type_tuple);
> - mi_field_int (uiout, -1, -1, ui_left, "nr_rows", nr_rows);
> - mi_field_int (uiout, -1, -1, ui_left, "nr_cols", nr_cols);
> - mi_open (uiout, "hdr", ui_out_type_list);
> + open (tblid, ui_out_type_tuple);

FYI, it's due to names like "open" potentially conflicting
with gnulib replacing global namespace C functions with
#define open rpl_open
that I'm looking at switching to use gnulib's C++ namespace
support. Luckily, looks like gnulib doesn't replace
"open" currently, so you don't need to wait for that:

$ grep -rn open | grep rpl_
stdio.in.h:191:# define fdopen rpl_fdopen
stdio.in.h:264:# define fopen rpl_fopen
stdio.in.h:388:# define freopen rpl_freopen
stdio.in.h:820:# define popen rpl_popen
dirent.in.h:79:# define opendir rpl_opendir
dirent.in.h:198:# define fdopendir rpl_fdopendir

> void
> -mi_table_end (struct ui_out *uiout)
> +mi_ui_out_impl::table_end ()
> {
> - mi_out_data *data = (mi_out_data *) ui_out_data (uiout);
> -
> - data->suppress_output = 0;
> - mi_close (uiout, ui_out_type_list); /* body */
> - mi_close (uiout, ui_out_type_tuple);
> + m_suppress_output = 0;
> + close (ui_out_type_list); /* body */
> + close (ui_out_type_tuple);

However:

$ grep -rn close | grep rpl_
stdio.in.h:172:# define fclose rpl_fclose
dirent.in.h:131:# define closedir rpl_closedir
unistd.in.h:303:# define close rpl_close

:-/

I'd like to post the gnulib namespace patch this week,
but I'm not sure I'll be able to.

> + virtual void field_string (int fldno, int width, ui_align align,
> + const char *fldname, const char *string) override;
> + virtual void ATTRIBUTE_PRINTF(6,0)
> + field_fmt (int fldno, int width, ui_align align, const char *fldname,
> + const char *format, va_list args) override;

It's more usual to put the ATTRIBUTE_PRINTFs at the end of the
declaration. Also, missing space before parens. Something like:

virtual void field_fmt (int fldno, int width, ui_align align,
const char *fldname,
const char *format, va_list args) override
ATTRIBUTE_PRINTF (6,0);

?

> + virtual void spaces (int numspaces) override;
> + virtual void text (const char *string) override;
> + virtual void ATTRIBUTE_PRINTF(2,0) message (const char *format, va_list args)
> + override;

Ditto. There are other instances in the patch. I won't point them all.

> +private:
> +
> + void field_separator ();
> + void open (const char *name, ui_out_type type);
> + void close (ui_out_type type);
> +
> + int m_suppress_field_separator;
> + int m_suppress_output;

bool ?

> + int m_mi_version;
> + std::vector<ui_file *> m_streams;
> +};

> -typedef struct tui_ui_out_data tui_out_data;
> +class tui_ui_out_impl : public cli_ui_out_impl
> +{
> +public:
> +
> + tui_ui_out_impl (ui_file *stream);

explicit.

> +
> + void field_int (int fldno, int width, ui_align align, const char *fldname,
> + int value) override;
> + void field_string (int fldno, int width, ui_align align, const char *fldname,
> + const char *string) override;
> + void ATTRIBUTE_PRINTF(6,0)
> + field_fmt (int fldno, int width, ui_align align, const char *fldname,
> + const char *format, va_list args) override;

attribute placement, space before parens.

> + void text (const char *string) override;
>

> + virtual int redirect (struct ui_file * outstream) = 0;
> +
> + /* Set as not MI-like by default. It is overridden in subclasses if
> + necessary. */
> +
> + virtual int is_mi_like_p ()
> + { return 0; }

bool ?

> +};
> +

Thanks,
Pedro Alves