Re: [PATCH v6] C++ify gdb/common/environ.c

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Monday, June 19 2017, Pedro Alves wrote:

> On 06/19/2017 06:59 PM, Sergio Durigan Junior wrote:
>> On Monday, June 19 2017, Pedro Alves wrote:
>> 
>>> On 06/19/2017 05:26 PM, Simon Marchi wrote:
>>>> On 2017-06-19 17:44, Pedro Alves wrote:
>>>>> If we take the "always push a NULL on construction" approach, and
>>>>> we want moved-from gdb_environs to be valid, then yes.  Note how this
>>>>> results in extra heap allocations when e.g., returning a
>>>>> gdb_environ from functions by value, and makes std::vector<gdb_environ>
>>>>> much less efficient when it decides it needs to reallocate/move
>>>>> elements.  Representing the empty state with a cleared internal
>>>>> vector would avoid this.
>>>>
>>>> Given the move case, since the goal is to be efficient, then yeah I
>>>> would agree
>>>> that it would make sense to make a little bit of efforts to avoid
>>>> allocating
>>>> memory for an objects we are almost certainly throwing away.
>>>>
>>>> But still, in order to leave environ objects in a valid state after a
>>>> move and
>>>> to pedantically comply with the STL spec which says that the vector is
>>>> left in
>>>> an unspecified state, shouldn't we do a .clear () on the moved-from
>>>> vector after
>>>> the move?
>>>
>>> See accepted answer at:
>>>
>>>  https://stackoverflow.com/questions/17730689/is-a-moved-from-vector-always-empty
>>>
>>> So the only case where it'd be needed would be in op=, and iff the 
>>> vectors had different allocators, which is not the case here.
>>> So no, it's not necessary.  But I'd be fine with calling it.
>>>
>>>>
>>>>> Note BTW, that we need to be careful with self-move leaving the
>>>>> *this object in a valid state.
>>>>
>>>> Should we just do
>>>>
>>>> if (&other == this)
>>>>   return *this;
>>>
>>> Might not be necessary if without that the object ends up
>>> valid anyway.  But what you wrote is a safe bet.
>> 
>> So, what do you guys think about the patch below, which applies on top
>> of the original?
>
> Missed fixing move ctor?
>
> +  /* Move constructor.  */
> +  gdb_environ (gdb_environ &&e)
> +    : m_environ_vector (std::move (e.m_environ_vector))
> +  {}

Indeed.  Fixed now.  I'll submit v7.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/