Re: [PATCH v2 5/5] Eliminate make_cleanup_ui_file_delete / make ui_file a class hierarchy

Simon Marchi
2017-01-24T19:11:37Z

On 2017-01-23 18:18, Pedro Alves wrote:
> diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
> index ea64220..cc14d9d 100644
> --- a/gdb/aix-thread.c
> +++ b/gdb/aix-thread.c
> @@ -1762,43 +1762,42 @@ aix_thread_extra_thread_info (struct target_ops
> *self,
> if (!PD_TID (thread->ptid))
> return NULL;
>
> - buf = mem_fileopen ();
> + string_file buf;

You did not remove the old declaration of buf.

>
> pdtid = thread->priv->pdtid;
> tid = thread->priv->tid;
>
> if (tid != PTHDB_INVALID_TID)
> /* i18n: Like "thread-identifier %d, [state] running, suspended"
> */
> - fprintf_unfiltered (buf, _("tid %d"), (int)tid);
> + buf.printf (_("tid %d"), (int)tid);
>
> status = pthdb_pthread_state (pd_session, pdtid, &state);
> if (status != PTHDB_SUCCESS)
> state = PST_NOTSUP;
> - fprintf_unfiltered (buf, ", %s", state2str (state));
> + buf.printf (", %s", state2str (state));
>
> status = pthdb_pthread_suspendstate (pd_session, pdtid,
> &suspendstate);
> if (status == PTHDB_SUCCESS && suspendstate == PSS_SUSPENDED)
> /* i18n: Like "Thread-Id %d, [state] running, suspended" */
> - fprintf_unfiltered (buf, _(", suspended"));
> + buf.printf (_(", suspended"));
>
> status = pthdb_pthread_detachstate (pd_session, pdtid,
> &detachstate);
> if (status == PTHDB_SUCCESS && detachstate == PDS_DETACHED)
> /* i18n: Like "Thread-Id %d, [state] running, detached" */
> - fprintf_unfiltered (buf, _(", detached"));
> + buf.printf (_(", detached"));
>
> pthdb_pthread_cancelpend (pd_session, pdtid, &cancelpend);
> if (status == PTHDB_SUCCESS && cancelpend)
> /* i18n: Like "Thread-Id %d, [state] running, cancel pending" */
> - fprintf_unfiltered (buf, _(", cancel pending"));
> + buf.printf (_(", cancel pending"));
>
> - ui_file_write (buf, "", 1);
> + buf.write ("", 1);
>
> xfree (ret); /* Free old buffer. */
>
> - ret = ui_file_xstrdup (buf, NULL);
> - ui_file_delete (buf);
> + ret = xstrdup (buf.string ());

Can xstrdup take an std::string? Implicit conversions only go the other
way I think.

It might be worth sending the series on the buildbot to test AIX.

>
> return ret;
> }
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 2bdfa57..bc170b1 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -9573,7 +9573,6 @@ extern initialize_file_ftype
> _initialize_arm_tdep; /* -Wmissing-prototypes */
> void
> _initialize_arm_tdep (void)
> {
> - struct ui_file *stb;
> long length;
> const char *setname;
> const char *setdesc;
> @@ -9645,13 +9644,12 @@ _initialize_arm_tdep (void)
> valid_disassembly_styles[num_disassembly_options] = NULL;
>
> /* Create the help text. */
> - stb = mem_fileopen ();
> - fprintf_unfiltered (stb, "%s%s%s",
> - _("The valid values are:\n"),
> - regdesc,
> - _("The default is \"std\"."));
> - helptext = ui_file_as_string (stb);
> - ui_file_delete (stb);
> + string_file stb;
> + stb.printf ("%s%s%s",
> + _("The valid values are:\n"),
> + regdesc,
> + _("The default is \"std\"."));
> + helptext = std::move (stb.string ());

I think having that static helptext variable is useless here. The help
text is copied in add_setshow_enum_cmd, so we don't need to keep
helptext for the whole lifetime of the program. We could pass stb.c_str
() directly to add_setshow_enum_cmd.

> +class string_file : public ui_file
> +{
> +public:
> + string_file () {}
> + ~string_file () override;
>
> -/* Returns a std::string containing the entire contents of FILE (as
> - determined by ui_file_put()). */
> -extern std::string ui_file_as_string (struct ui_file *file);
> + /* Override ui_file methods. */
>
> -/* Similar to ui_file_xstrdup, but return a new string allocated on
> - OBSTACK. */
> -extern char *ui_file_obsavestring (struct ui_file *file,
> - struct obstack *obstack, long *length);
> + void write (const char *buf, long length_buf) override;
>
> -extern long ui_file_read (struct ui_file *file, char *buf, long
> length_buf);
> + long read (char *buf, long length_buf) override
> + { gdb_assert_not_reached ("a string_file is not readable"); }
> +
> + /* string_file-specific public API. */
> +
> + /* Accesses the std::string containing the entire output collected
> + so far. Returns a non-const reference so that it's easy to move
> + the string contents out of the string_file. */

I didn't understand what you meant by "to move the string contents
out..." until I saw this

helptext = std::move (stb.string ());

If it had been "to std::move the string contents out..." it might have
been a bit clearer, I'm not entirely sure.

> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -187,18 +187,6 @@ make_cleanup_obstack_free (struct obstack
> *obstack)
> return make_cleanup (do_obstack_free, obstack);
> }
>
> -static void
> -do_ui_file_delete (void *arg)
> -{
> - ui_file_delete ((struct ui_file *) arg);
> -}
> -
> -struct cleanup *
> -make_cleanup_ui_file_delete (struct ui_file *arg)
> -{
> - return make_cleanup (do_ui_file_delete, arg);
> -}
> -

You can remove the declaration in utils.h for this.

That's all for me.

Thanks!

Simon