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

[Pedro Alves <palves@redhat.com>]

On 06/19/2017 08:18 AM, Simon Marchi wrote:
> On 2017-06-19 06:51, Sergio Durigan Junior wrote:
>> On Monday, June 19 2017, I wrote:
>>
>>> [...]
>>> -struct gdb_environ *
>>> -make_environ (void)
>>> +gdb_environ &
>>> +gdb_environ::operator= (gdb_environ &&e)
>>>  {
>>> -  struct gdb_environ *e;
>>> -
>>> -  e = XNEW (struct gdb_environ);
>>> -
>>> -  e->allocated = 10;
>>> -  e->vector = (char **) xmalloc ((e->allocated + 1) * sizeof (char *));
>>> -  e->vector[0] = 0;
>>> -  return e;
>>> -}
>>> -
>>> -/* Free an environment and all the strings in it.  */
>>> -
>>> -void
>>> -free_environ (struct gdb_environ *e)
>>> -{
>>> -  char **vector = e->vector;
>>> -
>>> -  while (*vector)
>>> -    xfree (*vector++);
>>> -
>>> -  xfree (e->vector);
>>> -  xfree (e);
>>> +  clear ();
>>> +  m_environ_vector = std::move (e.m_environ_vector);
>>> +  return *this;
>>>  }
>>
>> I should probably do an m_environ_vector.clear () before doing the
>> std::move, because the vector will contain the NULL element.  I'll make
>> sure to do this before I push the patch.
> 
> From what I saw stepping in the STL code, the previous content of the
> moved-to vector is cleared, as if you called .clear().  In
> _M_move_assign, it moves the old content to a temporary, stack allocated
> vector, which gets cleared when exiting that function.  So I think it's
> not necessary.

Right, m_environ_vector.clear() is not necessary.

Note that this move assignment (and likewise the move ctor) leaves the
source vector empty, which violates the "there's always a NULL entry
at the end" invariant.  That's OK if the only thing we want to support
of moved-from gdb_environ objects is destroying them, but please do
document that.

Otherwise, people assuming the standard library's rule, may be
confused/surprised, into thinking that this, e.g., should work:

gdb_environ env1;
env1.set ("VAR1", "value1");
gdb_environ env2;
env2 = std::move (env1);    // env1 has no NULL terminator after this.
env1.set ("VAR1", "value2); // whoops.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
17.6.5.15 Moved-from state of library types                                         [lib.types.movedfrom]

    Objects of types defined in the C++ standard library may be moved from (12.8).
    Move operations may be explicitly specified or implicitly generated. Unless
    otherwise specified, such moved-from objects shall be placed in a valid
    but unspecified state.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Thanks,
Pedro Alves