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

[Pedro Alves <palves@redhat.com>]

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))
+  {}

Thanks,
Pedro Alves