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

[Pedro Alves <palves@redhat.com>]

On 06/19/2017 04:30 PM, Simon Marchi wrote:
> On 2017-06-19 16:26, Pedro Alves wrote:
>> 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.
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> That's a good point.  We should definitely not let the environ object
> get in an invalid state.
> 
> Whatever the rule we choose for the terminating NULL, there exists some
> valid vector states which result in invalid environ states.  For
> example, an environ whose vector contains { NULL, NULL } is not valid. 
> Trying to set an env var in it would give { NULL, "FOO=BAR", NULL }, and
> that results in an unexpected environment array in the end.
> 
> Does that mean that after the vector move, we should make sure to leave
> the moved-from vector in a known state (i.e. clear it, and possible add
> a NULL), to make sure that we leave our environ object in a valid state?

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.

Note BTW, that we need to be careful with self-move leaving the
*this object in a valid state.

Thanks,
Pedro Alves