Re: [PATCH] gdb: make inferior::terminal a unique ptr

[Simon Marchi <simark@simark.ca>]

On 2020-06-25 2:09 p.m., Pedro Alves wrote:
> On 6/25/20 6:59 PM, Luis Machado wrote:
>> On 6/25/20 2:28 PM, Pedro Alves wrote:
>>> On 6/25/20 6:18 PM, Christian Biesinger via Gdb-patches wrote:
>>>> On Thu, Jun 25, 2020 at 10:55 AM Simon Marchi via Gdb-patches
>>>> <gdb-patches@sourceware.org> wrote:
>>>>> -    current_inferior ()->terminal = xstrdup (terminal_name);
>>>>> +    current_inferior ()->terminal.reset (xstrdup (terminal_name));
>>>>
>>>> Perhaps it really should be a std::string?
>>>
>>> I think there's no good reason for that.  It's a string that doesn't
>>> ever need to grow/shrink.  And then its use is to pass down to
>>> a syscall that accepts C-style null-terminated strings:
>>
>> I tend to agree with Christian. I suppose one positive outcome is to make the code less cumbersome. It's not like xstrdup is great to read anyway.
>>
>> Are we worried about performance implications of passing std::string's around like this?
> 
> It's more like -- why store a fat 32 bytes object in the inferior when
> a pointer does just as well.  There's no code that would benefit from
> making this a string, like, there's no strlen call that that would avoid.
> The code that wants to consume this wants a pointer.  A method-based
> interface like I pointed at in that github commit exposes a pointer.
> So it would just be implementation detail.
> 
> Thanks,
> Pedro Alves
> 

Heh, that's more noise than I expected for this patch :)

I tend to agree with Pedro on this.  I initially tried to replace it with
a gdb::optional<std::string>, then making get_inferior_io_terminal return a
reference to that... in the end it required changing a bunch of code in a way
that didn't really make it better.  The intent of this patch is just to get rid
of the manual free-ing, which is a concrete improvement.

Simon