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

[Pedro Alves <palves at redhat dot com>]

On 01/24/2017 07:11 PM, Simon Marchi wrote:

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

Meh.  No, that's broken.  I'll fix it.

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

I'll try building this on gcc119 (AIX 7.2) on the compile farm.
>From the discussion on gdb@, the bot is broken.  :-/

>> +  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.

OK.  While at it, this can actually be a string_printf
instead of a string_file.  I've made that change.

> 
>> +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.

That would read a bit odd to me, since "std::move" itself
does nothing, it's just a cast.  Maybe it'll be clearer with
an example?  Something like:

   /* 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.  */
+     so far.
+
+     Returns a non-const reference so that it's easy to move the
+     string contents out of the string_file.  E.g.:
+
+      string_file buf;
+      buf.printf (....");
+      buf.printf (....");
+      return std::move (buf.string ());
+  */
   std::string &string () { return m_string; }

> 
>> --- 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.

Indeed.  Fixed.

> 
> That's all for me.
> 
> Thanks!

Thank you!

-- 
Pedro Alves