This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v6] C++ify gdb/common/environ.c
- From: Pedro Alves <palves at redhat dot com>
- To: Simon Marchi <simon dot marchi at polymtl dot ca>, Sergio Durigan Junior <sergiodj at redhat dot com>
- Cc: GDB Patches <gdb-patches at sourceware dot org>
- Date: Mon, 19 Jun 2017 15:26:23 +0100
- Subject: Re: [PATCH v6] C++ify gdb/common/environ.c
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves at redhat dot com
- Dkim-filter: OpenDKIM Filter v2.11.0 mx1.redhat.com A9A8880460
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com A9A8880460
- References: <20170413040455.23996-1-sergiodj@redhat.com> <20170619043531.32394-1-sergiodj@redhat.com> <87k248y3zp.fsf@redhat.com> <8aabc6fabb04f4e3e8b08e6fa1b0eacc@polymtl.ca>
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