This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 19/22] Class-ify ui_out_impl
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.