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

Pedro Alves
2017-01-25T18:28:45Z

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