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

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

On 2016-11-30 08:09, Pedro Alves wrote:
>> +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.

Indeed it's not great.  This patchset is mostly about keeping the 
structure we have, but express it as C++.  So I'd argue that it uncovers 
something that already existed.  I think it makes the situation a tiny 
bit better though, as we have some type checking with dynamic_cast.  
With the static cast we have currently, if you happened to pass the 
wrong kind of object, it would probably fail catastrophically.

I'll consider working on merging ui_out and ui_out_impl_* in a single 
class hierarchy.  My first question is: what is a good pattern for the 
overlapping methods.  For example, table_begin.  We'll want to execute 
the version of the base class, which will then call the specialization.  
So we can't use the same name for both.  So, keep table_begin for the 
base class, and table_begin_impl for the derived classes?

>> +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.

Done.

>> +private:
>> +  void field_separator (void);
> 
> "(void)".  Probably easier to grep the whole patch set to
> find such cases and handle all of them.

Yep, that's what I did the first time I saw that comment.

>> +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.

And I guess it happens to work anyway for me because both the 
declaration, definition and usages get replaced?

Should I wait for your patch to get in (I'm not particularly in a 
hurry), or we can get it in despite "close" getting replaced?

>> +  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);
> 
> ?

Ah, I put them there because I though it was the only place where it 
worked when applied to virtual pure functions.  But apparently, this 
works:

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

I thought I had tried it and it didn't work, but I was wrong.  I'll fix 
them so they look like that.

>> +  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.

I'll go over all the patches and fix that.

>> +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 ?

Right.  I changed it, but from what I can see MI's m_suppress_output is 
never used (as in never set to true).  If that's indeed the case, I'll 
make a separate patch to remove it in the current master code.

CLI has a m_suppress_output that is used though, I changed it as well to 
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.

Done.

>> +
>> +  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.

Done.

>> +  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 ?

Done.

Thanks for the review.  I'll try to push the small patches that were 
OK'ed without comments, and re-post a complete v2 series.