This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2] Class-ify ui_out
On 2016-12-20 07:35, Pedro Alves wrote:
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);
Done.
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;
Done.
-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)
Done.
-/* 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.
Done.
-/* 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?
Should it return anything at all? Both implementations always return
0...
+
+ 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.
Done.
+ 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.
Indeed it would have been better. I didn't think about that, sorry.
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.
Done.
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, ...)
Indeed, there were many instances in cli-out.c, tui-out.c and mi-out.c.
~~~~~~~~~~~~~~~~~~~~~~~~
/* 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),
I looked up all usages of m_suppress_field_separator and
m_suppress_output to make sure they were using boolean.
Should return bool (all implementations of course):
int
-ui_out_is_mi_like_p (struct ui_out *uiout)
+ui_out::is_mi_like_p ()
Done.
Otherwise it looks good to me. Thanks much for doing this!
Please push.
Thanks for reviewing, I'm sure it wasn't one of the most pleasant to
do...
I'll give it a last go on the buildbot and push if it's all good.
Simon