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

Simon Marchi
2016-11-30T22:38:06Z

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.