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

Pedro Alves
2017-01-25T17:52:45Z

On 01/24/2017 06:29 PM, Simon Marchi wrote:
> On 2017-01-23 18:18, Pedro Alves wrote:
>> @@ -195,20 +188,18 @@ handle_redirections (int from_tty)
>> return;
>> }
>>
>> - output = gdb_fopen (logging_filename, logging_overwrite ? "w" : "a");
>> - if (output == NULL)
>> + std::unique_ptr<stdio_file> log (new stdio_file ());
>
> What is the "rule" for using std::unique_ptr directly vs defining a
> typedef?
>
> typedef std::unique_ptr<stdio_file> stdio_file_up;

I don't think there's one. I was going by "if we're going to
be typing the long std::unique_ptr<whatnot> all other the place,
add a typedef". I think that for a single use or two, requiring
a typedef may be excessive?

The original "rule" I proposed was about the "_up" naming. I
was originally thinking of preventing a proliferation of a mix
of "_uptr" vs "_up" vs "UP" vs "_whatnot" suffixes.

>
> I'd like if we could avoid having a mix of styles, unless there are good
> reasons for it.

No real good reason other than "I hadn't bothered" :-)
I've added the typedef locally now.

>
>> + if (!log->open (logging_filename, logging_overwrite ? "w" : "a"))
>> perror_with_name (_("set logging"));
>> - cleanups = make_cleanup_ui_file_delete (output);
>> +
>> + output = log.get ();
>>
>> /* Redirects everything to gdb_stdout while this is running. */
>> if (!logging_redirect)
>> {
>> no_redirect_file = output;
>>
>> - output = tee_file_new (gdb_stdout, 0, no_redirect_file, 0);
>> - if (output == NULL)
>> - perror_with_name (_("set logging"));
>> - make_cleanup_ui_file_delete (output);
>> + output = new tee_file (gdb_stdout, 0, no_redirect_file, 0);
>
> Is there anything that replaces this cleanup? IOW, if the
> fprinf_unfiltered just below did throw an exception for some reason,
> would this tee_file leak?

Looks like there isn't. I'll fix it. Bah, this is going to
mess up a follow-up patch to push down the tee to the interpreter
callback that I had prepared meanwhile. :-)

>> - return code;
>> + add_code_footer (inst->scope, &buf);
>> + return std::move (buf.string ());
>
> I would have thought that this std::move would be superfluous, because
> the compiler would do it anyway. Is it the case? Is it a good practice
> to use move explicitly to make sure it's a move and not a copy (and
> probably get a compile-time error if a move is not possible)?

It's actually usually a bad practice, since if you do:

std::string foo ()
{
std::string str;
return std::move (std);
}

you prevent the compiler from eliding the "str" local.

In this case, "buf.string ()" is a reference, so if we don't
tell the compiler to treat it as an rvalue reference,
then the compiler won't know that it can move the contents
out of the buffer. I.e., call the move constructor on the
caller's string directly instead of the copy constructor (and
doing a deep copy).

>>
>> -static void
>> -ioscm_file_port_put (struct ui_file *file,
>> - ui_file_put_method_ftype *write,
>> - void *dest)
>> +void
>> +ioscm_file_port::flush ()
>> {
>> - ioscm_file_port *stream = (ioscm_file_port *) ui_file_data (file);
>> -
>> - if (stream->magic != &file_port_magic)
>> - internal_error (__FILE__, __LINE__,
>> - _("ioscm_file_port_put: bad magic number"));
>> -
>> - /* This function doesn't meld with ports very well. */
>> }
>
> It looks like there's a low-level function, scm_force_output, that
> "flushes" a port. I wonder if we'd want to do that in flush.

Sounds like it (though I've not added it).

> mi_console_file::puts looks identical to ui_file::puts. Is there a
> reason to overload it?

Looks like no reason. Good catch.

> Throughout mi_cmd_data_read_memory, is is worth it performance-wise to
> re-use the same string_file and doing some .clear() rather than
> declaring different ones in more specific scopes? Since they are stack
> allocated, there shouldn't be any runtime cost in execution time. Which
> is faster between constructing a string_file and calling .clear()?

Even though the string_buffer is stack allocated, the contents of
its contents (the internal buffer inside std::string) are not.
If we moved the stream local to inside the loop scope, that'd force
allocating and growing the std::string's internals from scratch in
each iteration. With the clear() approach, we reuse the same buffer
on all iterations. "std::string::clear()" only sets the string's data
size to 0, it does not reduce the string's capacity, unlike
"std::string::shrink_to_fit()".

> I think it would be less error-prone (like forgetting to call .clear()
> between two usages) to declare multiple string_file objects in the
> scopes that need them, but I understand if you'd rather not do it for
> performance reasons. I also understand if you say that it's because
> it's the way it worked and don't want to spend time on a deprecated
> function anyway :).

All the above. :-)

>
>> /* Expand tabs into spaces, since ncurses on MS-Windows doesn't. */
>> - ret = tui_expand_tabs (p, 0);
>> + ret = tui_expand_tabs (str.c_str (), 0);
>
> It's not in the scope of this patch, but it would be nice to rewrite
> tui_expand_tabs using C++ features, I'm sure it could be much simpler.

Yeah, I'm sure it would. When I look at such things, my first
thought is "where is the string going to end up stored? Would it
be reasonable to cascade std::string out of the function
across several layers in order to avoid the xstrdup that using
std::string internally but returning char * would require?".

>
>
> I'm not done looking at the patch yet, but I've deleted to much of the
> quote and I have comments about stuff I deleted, so it will be simpler
> to just send another e-mail.

Thanks,
Pedro Alves