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

  Changes from v5:

- Initializing vector on ctor with NULL element;

- Expanded unittest to make sure we can set/unset an environment
  variable even with an empty gdb_environ.

As part of the preparation necessary for my upcoming task, I'd like to
propose that we turn gdb_environ into a class.  The approach taken
here is simple: the class gdb_environ contains everything that is
needed to manipulate the environment variables.  These variables are
stored in an std::vector<char *>, which can be converted to a 'char
**' and passed as argument to functions that need it.

The usage has not changed much.  As per Pedro's suggestion, this class
uses a static factory method initialization.  This means that when an
instance is created, it is initially empty.  When needed, it has to be
initialized using the static method 'from_host_environ'.

As mentioned before, this is a preparation for an upcoming work that I
will be posting in the next few weeks or so.  For that work, I'll
probably create another data structure that will contain all the
environment variables that were set by the user using the 'set
environment' command, because I'll need access to them.  This will be
much easier with the class-ification of gdb_environ.

As noted, this has been regression-tested with the new version of
environ.exp and no regressions were found.

gdb/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>

	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
	'unittests/environ-selftests.c'.
	(SUBDIR_UNITTESTS_OBS): Add 'environ-selftests.o'.
	* charset.c (find_charset_names): Declare object 'iconv_env'.
	Update code to use 'iconv_env' object.  Remove call to
	'free_environ'.
	* common/environ.c: Include <utility>.
	(make_environ): Delete function.
	(free_environ): Delete function.
	(gdb_environ::clear): New function.
	(gdb_environ::operator=): New function.
	(gdb_environ::get): Likewise.
	(environ_vector): Delete function.
	(set_in_environ): Delete function.
	(gdb_environ::set): New function.
	(unset_in_environ): Delete function.
	(gdb_environ::unset): New function.
	(gdb_environ::envp): Likewise.
	* common/environ.h: Include <vector>.
	(struct gdb_environ): Delete; transform into...
	(class gdb_environ): ... this class.
	(free_environ): Delete prototype.
	(init_environ, get_in_environ, set_in_environ, unset_in_environ,
	environ_vector): Likewise.
	* infcmd.c (run_command_1): Update code to call
	'envp' from 'gdb_environ' class.
	(environment_info): Update code to call methods from 'gdb_environ'
	class.
	(unset_environment_command): Likewise.
	(path_info): Likewise.
	(path_command): Likewise.
	* inferior.c (inferior::~inferior): Delete call to 'free_environ'.
	(inferior::inferior): Initialize 'environment' using the host's
	information.
	* inferior.h: Remove forward declaration of 'struct gdb_environ'.
	Include "environ.h".
	(class inferior) <environment>: Change type from 'struct
	gdb_environ' to 'gdb_environ'.
	* mi/mi-cmd-env.c (mi_cmd_env_path): Update code to call
	methods from 'gdb_environ' class.
	* solib.c (solib_find_1): Likewise
	* unittests/environ-selftests.c: New file.

gdb/gdbserver/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>

	* linux-low.c (linux_create_inferior): Adjust code to access the
	environment information via 'gdb_environ' class.
	* lynx-low.c (lynx_create_inferior): Likewise.
	* server.c (our_environ): Make it an instance of 'gdb_environ'.
	(get_environ): Return a pointer to 'our_environ'.
	(captured_main): Initialize 'our_environ'.
	* server.h (get_environ): Adjust prototype.
	* spu-low.c (spu_create_inferior): Adjust code to access the
	environment information via 'gdb_environ' class.
---
 gdb/Makefile.in                   |   2 +
 gdb/charset.c                     |  11 +--
 gdb/common/environ.c              | 199 ++++++++++++++------------------------
 gdb/common/environ.h              |  65 +++++++++----
 gdb/gdbserver/linux-low.c         |   2 +-
 gdb/gdbserver/lynx-low.c          |   2 +-
 gdb/gdbserver/server.c            |   9 +-
 gdb/gdbserver/server.h            |   6 +-
 gdb/gdbserver/spu-low.c           |   2 +-
 gdb/infcmd.c                      |  35 +++----
 gdb/inferior.c                    |   4 +-
 gdb/inferior.h                    |   6 +-
 gdb/mi/mi-cmd-env.c               |   6 +-
 gdb/solib.c                       |   7 +-
 gdb/unittests/environ-selftests.c |  89 +++++++++++++++++
 15 files changed, 252 insertions(+), 193 deletions(-)
 create mode 100644 gdb/unittests/environ-selftests.c
  

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.

Thanks,
  

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.

Simon
  

On 06/19/2017 05:35 AM, Sergio Durigan Junior wrote:
> +private:
> +  /* A vector containing the environment variables.  This is useful
> +     for when we need to obtain a 'char **' with all the existing
> +     variables.  */
> +  std::vector<char *> m_environ_vector;
> +};

This "This is useful" comment doesn't seem to make much
sense here in isolation.  What exactly is useful, and in comparison
to what else?  Maybe you're referring to the choice of type of element
in the vector, say vs a unique_ptr.  Please clarify the comment.  As
is, it would sound like a comment more fit to the class'es intro
or to the envp() method.

On 06/19/2017 05:35 AM, Sergio Durigan Junior wrote:
>    else
>      {
> -      char **vector = environ_vector (current_inferior ()->environment);
> +      char **envp = current_inferior ()->environment.envp ();
>  
> -      while (*vector)
> -	{
> -	  puts_filtered (*vector++);
> -	  puts_filtered ("\n");
> -	}
> +      if (envp != NULL)

I suspect this NULL check here was only needed in the previous
version that mishandled empty environs.  I can't see how it
makes sense now.  If you still need it, then there's a bug
elsewhere.

> +	for (int idx = 0; envp[idx] != NULL; ++idx)
> +	  {
> +	    puts_filtered (envp[idx]);
> +	    puts_filtered ("\n");
> +	  }
>      }



> +  if (setenv ("GDB_SELFTEST_ENVIRON", "1", 1) != 0)
> +    error ("Could not set environment variable for testing.");

Missing _() around error's format string.




> +
> +  gdb_environ env;
> +
> +  SELF_CHECK (env.envp ()[0] == NULL);
> +
> +  SELF_CHECK (env.get ("PWD") == NULL);
> +  env.set ("PWD", "test");
> +  env.unset ("PWD");
> +

Please add another

  SELF_CHECK (env.envp ()[0] == NULL);

after the unset.  I didn't spot any check making sure
that invariant holds after an unset.


Thanks,
Pedro Alves
  

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
  

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?
  

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
  

On 06/19/2017 04:44 PM, Pedro Alves wrote:

> Note how this
> results in extra heap allocations when e.g., returning a
> gdb_environ from functions by value,

Bah, unless RVO, or C++17 guaranteed copy-elision, of course.
This particular sentence here is a bit misleading, sorry.

Thanks,
Pedro Alves
  

On Monday, June 19 2017, Pedro Alves wrote:

> On 06/19/2017 05:35 AM, Sergio Durigan Junior wrote:
>> +private:
>> +  /* A vector containing the environment variables.  This is useful
>> +     for when we need to obtain a 'char **' with all the existing
>> +     variables.  */
>> +  std::vector<char *> m_environ_vector;
>> +};
>
> This "This is useful" comment doesn't seem to make much
> sense here in isolation.  What exactly is useful, and in comparison
> to what else?  Maybe you're referring to the choice of type of element
> in the vector, say vs a unique_ptr.  Please clarify the comment.  As
> is, it would sound like a comment more fit to the class'es intro
> or to the envp() method.

This is probably a leftover comment from a very early version.  I
removed the part about usefulness.

> On 06/19/2017 05:35 AM, Sergio Durigan Junior wrote:
>>    else
>>      {
>> -      char **vector = environ_vector (current_inferior ()->environment);
>> +      char **envp = current_inferior ()->environment.envp ();
>>  
>> -      while (*vector)
>> -	{
>> -	  puts_filtered (*vector++);
>> -	  puts_filtered ("\n");
>> -	}
>> +      if (envp != NULL)
>
> I suspect this NULL check here was only needed in the previous
> version that mishandled empty environs.  I can't see how it
> makes sense now.  If you still need it, then there's a bug
> elsewhere.

No, it is not needed anymore.  Removed.

>> +	for (int idx = 0; envp[idx] != NULL; ++idx)
>> +	  {
>> +	    puts_filtered (envp[idx]);
>> +	    puts_filtered ("\n");
>> +	  }
>>      }
>
>
>
>> +  if (setenv ("GDB_SELFTEST_ENVIRON", "1", 1) != 0)
>> +    error ("Could not set environment variable for testing.");
>
> Missing _() around error's format string.

Fixed.

>> +
>> +  gdb_environ env;
>> +
>> +  SELF_CHECK (env.envp ()[0] == NULL);
>> +
>> +  SELF_CHECK (env.get ("PWD") == NULL);
>> +  env.set ("PWD", "test");
>> +  env.unset ("PWD");
>> +
>
> Please add another
>
>   SELF_CHECK (env.envp ()[0] == NULL);
>
> after the unset.  I didn't spot any check making sure
> that invariant holds after an unset.

This invariant is not supposed to hold after every unset, only after a
clear or after an unset that removes the only variable in the vector.

Thanks,
  

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?

> 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;

?
  

On 06/19/2017 05:13 PM, Sergio Durigan Junior wrote:
> On Monday, June 19 2017, Pedro Alves wrote:

>>> +
>>> +  gdb_environ env;
>>> +
>>> +  SELF_CHECK (env.envp ()[0] == NULL);
>>> +
>>> +  SELF_CHECK (env.get ("PWD") == NULL);
>>> +  env.set ("PWD", "test");
>>> +  env.unset ("PWD");
>>> +
>>
>> Please add another
>>
>>   SELF_CHECK (env.envp ()[0] == NULL);
>>
>> after the unset.  I didn't spot any check making sure
>> that invariant holds after an unset.
> 
> This invariant is not supposed to hold after every unset, only after a
> clear or after an unset that removes the only variable in the vector.

... which is exactly the case above.  And for unsets where there are
still elements, the invariant is that the last element is NULL
[which is of course the same invariant].  So maybe we should have a
little function like this (could reuse countargv too):

static size_t
countenvp (const gdb_environ &env)
{
   char **envp = env.envp ();
   size_t count = 0;
   while (envp[count] != NULL)
     count++;
   return count;
}

Used instead of the NULL SELF_CHECKs, like:

  gdb_environ env;

  /* This makes sure that env.envp() is NULL terminated.  */
  SELF_CHECK (countenvp (env) == 0);

  /* ENV is empty, so we shouldn't be able to find any var.  */
  SELF_CHECK (env.get ("PWD") == NULL);

  /* Set a var, and make sure that env.envp() is still NULL
     terminated.  */
  env.set ("PWD", "test");
  SELF_CHECK (countenvp (env) == 1);

  /* Clear the var and make sure that env.envp() is left NULL
     terminated when we remove the last element.  */
  env.unset ("PWD");
  SELF_CHECK (countenvp (env) == 0);

etc.

I find that adding guiding comments like above helps, btw.

Thanks,
Pedro Alves
  

On Monday, June 19 2017, Pedro Alves wrote:

> On 06/19/2017 05:13 PM, Sergio Durigan Junior wrote:
>> On Monday, June 19 2017, Pedro Alves wrote:
>
>>>> +
>>>> +  gdb_environ env;
>>>> +
>>>> +  SELF_CHECK (env.envp ()[0] == NULL);
>>>> +
>>>> +  SELF_CHECK (env.get ("PWD") == NULL);
>>>> +  env.set ("PWD", "test");
>>>> +  env.unset ("PWD");
>>>> +
>>>
>>> Please add another
>>>
>>>   SELF_CHECK (env.envp ()[0] == NULL);
>>>
>>> after the unset.  I didn't spot any check making sure
>>> that invariant holds after an unset.
>> 
>> This invariant is not supposed to hold after every unset, only after a
>> clear or after an unset that removes the only variable in the vector.
>
> ... which is exactly the case above.  And for unsets where there are
> still elements, the invariant is that the last element is NULL
> [which is of course the same invariant].  So maybe we should have a
> little function like this (could reuse countargv too):

Yes, I know, I was just correcting the last part of your sentence:

  I didn't spot any check making sure that invariant holds after *an*
  unset.

(emphasis mine)

> static size_t
> countenvp (const gdb_environ &env)
> {
>    char **envp = env.envp ();
>    size_t count = 0;
>    while (envp[count] != NULL)
>      count++;
>    return count;
> }
>
> Used instead of the NULL SELF_CHECKs, like:
>
>   gdb_environ env;
>
>   /* This makes sure that env.envp() is NULL terminated.  */
>   SELF_CHECK (countenvp (env) == 0);
>
>   /* ENV is empty, so we shouldn't be able to find any var.  */
>   SELF_CHECK (env.get ("PWD") == NULL);
>
>   /* Set a var, and make sure that env.envp() is still NULL
>      terminated.  */
>   env.set ("PWD", "test");
>   SELF_CHECK (countenvp (env) == 1);
>
>   /* Clear the var and make sure that env.envp() is left NULL
>      terminated when we remove the last element.  */
>   env.unset ("PWD");
>   SELF_CHECK (countenvp (env) == 0);
>
> etc.
>
> I find that adding guiding comments like above helps, btw.

Right, I'll add guiding comments.

Thanks,
  

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.

Thanks,
Pedro Alves