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

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

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