Re: move constructors likely a requirement

[Rik]

On 08/21/2019 01:23 PM, John W. Eaton wrote:
> On 8/20/19 6:59 PM, Rik wrote:
>
>> However, there is no
>> decrease in the number of times (9) the ~octave_value_destructor is called
>> because I couldn't find a clean way to "zero out" the rvalue object that is
>> being moved.
>
>>      std::swap (m_data, obj.m_data);
>>      std::swap (m_names, obj.m_names);
>
> I think these should be
>
>   m_data = std::move (obj.m_data);
>   m_names = std::move (obj.m_names);
>
> and not calls to std::swap.

I made them std::swap because I couldn't figure out how to easily zero out
the std::vector<octave_value> that is a data member of octave_value_list. 
If an instance of octave_value_list already has data in it, and move
assignment is called, then the existing data has to be dealt with in some
manner.  It was only an idea, but I swapped the data from the existing
object with the incoming data from the rvalue temporary.  This accomplished
the goal of having the incoming data supplant the existing data, and when
the rvalue object goes out of scope it calls the destructor which takes
care of clearing any data that was already present.  The move constructor
in your diffs is

+  octave_value_list& operator = (octave_value_list&& obj)
+  {
+    if (this != &obj)
+      {
+        m_data = std::move (obj.m_data);
+        m_names = std::move (obj.m_names);
+      }
+
+    return *this;
+  }

Wouldn't you need a call such as "m_data.clear ();" to get rid of any
existing data before assigning to it?

>
> But in any case, whether a copy or move happens, my understanding is that
> the destructor for the moved object will still be called, but it doesn't
> have to do anything.

I wrote some test code to verify and the destructor for the rvalue object
is always called.

> For example, for a reference-counted class like octave_value, you would
> do something like
>
>   octave_value (octave_value&& other)
>     : rep (other.rep)
>   {
>     other.rep = nullptr;
>   }
>
> to transfer the ownership of REP from the OTHER to *THIS instead of
> manipulating the reference count.  Then the destructor should also be
> changed so that it will skip decrementing the reference count if REP is
> nullptr:
>
>   ~octave_value (void)
>   {
>     if (rep && --rep->count == 0)
>       delete rep;
>   }
>
> Currently, we just have "if (--rep->count == 0)" but that will fail if we
> introduce a move constructor.
>

This is also how I understand "zeroing out" objects that use pointers.


> Instead of looking at whether the destructor is called, maybe it would be
> better to trace how many times atomic variables are accessed and modified?

I think that is what needs to be monitored.


>
> I have been experimenting with the attached patches:
>
>   diffs-1.txt: Change the string_vector class so it contains an
> Array<std::string> object instead of deriving from Array<std::string>
> (next step is to use some other container instead of the heavyweight
> Array<T>).
>
>   diffs-2.txt: Introduce move constructors and assignment operators for
> the Array, string_vector and octave_value_list classses.
>
>   diffs-3.txt: Improve the tree_evaluator::convert_to_const_vecctor
> function so that it doesn't create as many octave_value_list objects.
>
> I think it is worth attempting to add move constructors and assignment
> operators to any classes we define that need to be copied.  As we do for
> copy constructors and assignment operators, it would also make sense to
> use the "= default" specification where possible so that the compiler can
> generate these functions, or to use "= delete" to explicitly state that
> they should not be defined.

This article on Stack Overflow discusses how the Rule of 3 has now become
more like the Rule of 5
(https://stackoverflow.com/questions/4782757/rule-of-three-becomes-rule-of-five-with-c11?noredirect=1).
 
If we have classes which are complex enough to require one or more
non-default constructors then we should (as of C++11) be providing the
other constructors or indicating to the compiler what we wish to have happen. 

Interestingly, I think we could use '= delete' to disable move constructors
and then use the compiler to find locations in the code base that would use
a move constructor.  At that point could decide whether to write a move
constructor for the particular class, or re-structure the API for the
function such that it is passed a reference to a class object to work on.

--Rik