Re: [PATCH v2] Class-ify ui_out

[Pedro Alves <palves at redhat dot com>]

On 12/12/2016 04:15 PM, Simon Marchi wrote:

> -static void
> -cli_table_begin (struct ui_out *uiout, int nbrofcols,
> -		 int nr_rows,
> -		 const char *tblid)
> +void
> +cli_ui_out::do_table_begin (int nbrofcols, int nr_rows, const char *tblid)
>  {
> -  cli_out_data *data = (cli_out_data *) ui_out_data (uiout);
> -
>    if (nr_rows == 0)
> -    data->suppress_output = 1;
> +    m_suppress_output = true;
>    else
>      /* Only the table suppresses the output and, fortunately, a table
>         is not a recursive data structure.  */
> -    gdb_assert (data->suppress_output == 0);
> +    gdb_assert (m_suppress_output == 0);

    gdb_assert (!m_suppress_output);

> index dfa96d6..3e6a2fd 100644
> --- a/gdb/cli/cli-interp.c
> +++ b/gdb/cli/cli-interp.c
> @@ -34,7 +34,7 @@
>  struct cli_interp
>  {
>    /* The ui_out for the console interpreter.  */
> -  struct ui_out *cli_uiout;
> +  cli_ui_out *cli_uiout;
>  };

Exactly.  Nice.


> -static void
> -mi_open (struct ui_out *uiout, const char *name, enum ui_out_type type)
> +void
> +mi_ui_out::open (const char *name, ui_out_type type)
>  {
> -  mi_out_data *data = (mi_out_data *) ui_out_data (uiout);
> -  ui_file *stream = data->streams.back ();
> +  ui_file *stream = m_streams.back ();
> +
> +  field_separator ();
> +  m_suppress_field_separator = 1;

= true;

>  
> -  field_separator (uiout);
> -  data->suppress_field_separator = 1;
>    if (name)
>      fprintf_unfiltered (stream, "%s=", name);
> +
>    switch (type)
>      {
>      case ui_out_type_tuple:
>        fputc_unfiltered ('{', stream);
>        break;
> +
>      case ui_out_type_list:
>        fputc_unfiltered ('[', stream);
>        break;
> +
>      default:
>        internal_error (__FILE__, __LINE__, _("bad switch"));
>      }
>  }
>  


> -static void
> -mi_out_data_ctor (mi_out_data *self, int mi_version, struct ui_file *stream)
> +mi_ui_out::mi_ui_out (int mi_version, ui_file *stream)
> +: m_suppress_field_separator (0),

m_suppress_field_separator (false)

> +  m_suppress_output (false),
> +  m_mi_version (mi_version)
>  {
>    gdb_assert (stream != NULL);
>  
> -  self->streams.push_back (stream);
> +  m_streams.push_back (stream);
> +}
>  
> -  self->suppress_field_separator = 0;
> -  self->mi_version = mi_version;
> +mi_ui_out::~mi_ui_out ()
> +{
>  }
>  


> -/* Initialize private members at startup.  */
> +/* Helper function to return the given UIOUT as an mi_ui_out.  It is an error
> +   to call this fucntion with an ui_out that is not an MI.  */

typo: fucntion.

>  
> -struct ui_out *
> -mi_out_new (int mi_version)
> +static mi_ui_out *
> +as_mi_ui_out (ui_out *uiout)
>  {
> -  ui_out_flags flags = 0;
> -  mi_out_data *data = new mi_out_data ();
> -  struct ui_file *stream = mem_fileopen ();
> +  mi_ui_out *mi_uiout = dynamic_cast<mi_ui_out *> (uiout);
> +
> +  gdb_assert (mi_uiout != NULL);
> +
> +  return mi_uiout;
> +}
>  
> -  mi_out_data_ctor (data, mi_version, stream);
> -  return ui_out_new (&mi_ui_out_impl, data, flags);
> +int
> +mi_version (ui_out *uiout)
> +{
> +  return as_mi_ui_out (uiout)->version ();
> +}


> -/* Return the version number of the current MI.  */
> -extern int mi_version (struct ui_out *uiout);
> +class mi_ui_out : public ui_out
> +{
> +public:
> +
> +  explicit mi_ui_out (int mi_version, ui_file *stream);
> +  virtual ~mi_ui_out ();
> +
> +  /* MI-specific */
> +  void rewind ();
> +  void put (struct ui_file *stream);
> +
> +  /* Return the version number of the current MI.  */
> +  int version ();
> +
> +protected:
> +
> +  virtual void do_table_begin (int nbrofcols, int nr_rows, const char *tblid)
> +    override;
> +  virtual void do_table_body () override;
> +  virtual void do_table_header (int width, ui_align align,
> +			     const std::string &col_name,
> +			     const std::string &col_hdr) override;
> +  virtual void do_table_end () override;
> +
> +  virtual void do_begin (ui_out_type type, const char *id) override;
> +  virtual void do_end (ui_out_type type) override;
> +  virtual void do_field_int (int fldno, int width, ui_align align,
> +			  const char *fldname, int value) override;
> +  virtual void do_field_skip (int fldno, int width, ui_align align,
> +			   const char *fldname) override;
> +  virtual void do_field_string (int fldno, int width, ui_align align,
> +			     const char *fldname, const char *string) override;
> +  virtual void do_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 do_spaces (int numspaces) override;
> +  virtual void do_text (const char *string) override;
> +  virtual void do_message (const char *format, va_list args) override
> +    ATTRIBUTE_PRINTF (2,0) ;
> +  virtual void do_wrap_hint (const char *identstring) override;
> +  virtual void do_flush () override;
> +  virtual int do_redirect (struct ui_file * outstream) override;

Should do_redirect return bool?

> +
> +  virtual bool do_is_mi_like_p () override
> +  { return true; }
> +
> +private:
> +
> +  void field_separator ();
> +  void open (const char *name, ui_out_type type);
> +  void close (ui_out_type type);
> +
> +  int m_suppress_field_separator;

bool.

> +  bool m_suppress_output;
> +  int m_mi_version;
> +  std::vector<ui_file *> m_streams;
> +};
> +
> +mi_ui_out *mi_out_new (int mi_version);
> +int mi_version (ui_out *uiout);
> +void mi_out_put (ui_out *uiout, struct ui_file *stream);
> +void mi_out_rewind (ui_out *uiout);
>  
>  #endif /* MI_OUT_H */

I'm finding this patch very hard to skim through.  :-(
Mainly because it mixes changing the implementation of the ui_out
types, and elimination of all the ui_out_field_XXX etc. functions.
I.e, I'm finding myself scrolling through the latter all again,
while it probably didn't change vs v1, making the C++-fyication
of the ui-out types hard to see among a forest of mechanical hunks.

I think it'd have been much easier to review if the patch
was split in two patches: one that changes ui-out.h|.c,
cli-out.h|c, mi-out.h|c and tui-out.h|c, leaving the 
ui_out_field_XXX public API in place, and then another
mostly mechanical one that eliminated the ui_out_field_XXX
public API and its uses throughout.

So at this point I applied the patch locally, and diffed
the *-out.h files.  Hopefully I won't miss anything
important.

BTW, git am showed:

$ git am /tmp/simon.mbox
Applying: Class-ify ui_out
.../src/.git/rebase-apply/patch:575: trailing whitespace.
      uiout->field_string ("reason", 
.../src/.git/rebase-apply/patch:949: trailing whitespace.
    uiout->field_fmt ("enabled", "%c", 
.../src/.git/rebase-apply/patch:2866: trailing whitespace.
      uiout->field_string ("min-prot", 
.../src/.git/rebase-apply/patch:2869: trailing whitespace.
      uiout->field_string ("max-prot", 
.../src/.git/rebase-apply/patch:3401: trailing whitespace.
        uiout->field_string ("reason", 
warning: 5 lines add whitespace errors.



I noticed several instances of missing reindentation of
second line in method implementation prototypes, like:

void
cli_ui_out::out_field_fmt (int fldno, const char *fldname,
				const char *format, ...)


~~~~~~~~~~~~~~~~~~~~~~~~
 /* access to ui_out format private members */
 -
 -static void
 -field_separator (struct ui_out *uiout)
 +void
 +mi_ui_out::field_separator ()
  {
 -  mi_out_data *data = (mi_out_data *) ui_out_data (uiout);
 -  ui_file *stream = data->streams.back ();
 -
 -  if (data->suppress_field_separator)
 -    data->suppress_field_separator = 0;
 +  if (m_suppress_field_separator)
 +    m_suppress_field_separator = 0;
    else
 -    fputc_unfiltered (',', stream);
 +    fputc_unfiltered (',', m_streams.back ());
  }
~~~~~~~~~~~~~~~~~~~~~~~~

Above should be "m_suppress_field_separator = false;"

Same in mi_ui_out::close, and also in:

 +mi_ui_out::mi_ui_out (int mi_version, ui_file *stream)
 +: m_suppress_field_separator (0),

Should return bool (all implementations of course):

  int
 -ui_out_is_mi_like_p (struct ui_out *uiout)
 +ui_out::is_mi_like_p ()

Otherwise it looks good to me.  Thanks much for doing this!

Please push.

Pedro Alves