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

[Pedro Alves <palves@redhat.com>]

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