Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.c++ > #70246 > unrolled thread

I'm not able to find the error

Back to article view | Back to comp.lang.c++

Contents

  I'm not able to find the error alexo <alelvb@inwind.it> - 2019-12-04 17:54 +0100
    Re: I'm not able to find the error Volker Wippel <Volker.Wippel@v2c2RemoveNoSpam.at> - 2019-12-04 18:40 +0100
    Re: I'm not able to find the error Daniel <danielaparker@gmail.com> - 2019-12-04 10:01 -0800
      Re: I'm not able to find the error Daniel <danielaparker@gmail.com> - 2019-12-04 10:06 -0800
    Re: I'm not able to find the error Manfred <noname@add.invalid> - 2019-12-04 20:16 +0100
      Re: I'm not able to find the error Manfred <invalid@add.invalid> - 2019-12-05 01:42 +0100
        Re: I'm not able to find the error alexo <alelvb@inwind.it> - 2019-12-05 03:45 +0100
          Re: I'm not able to find the error alexo <alelvb@inwind.it> - 2019-12-05 03:58 +0100
            Re: I'm not able to find the error Mike Terry <news.dead.person.stones@darjeeling.plus.com> - 2019-12-05 04:10 +0000
              Re: I'm not able to find the error alexo <alelvb@inwind.it> - 2019-12-05 05:45 +0100
            Re: I'm not able to find the error Manfred <noname@add.invalid> - 2019-12-05 17:29 +0100
    Re: I'm not able to find the error alexo <alelvb@inwind.it> - 2019-12-04 22:52 +0100
      Re: I'm not able to find the error Daniel <danielaparker@gmail.com> - 2019-12-04 16:47 -0800
        Re: I'm not able to find the error alexo <alelvb@inwind.it> - 2019-12-05 03:37 +0100

#70246 — I'm not able to find the error

Hello guys,
I've written this code to get acquainted to assignment operators,
move constructors and move assignment operators, but I got a
strange behavior in my code:
I report it in its fullness because it's 123 lines:

// code starts here
#include <iostream>

using std::cout;
using std::ostream;

class Vector
{
     friend ostream& operator<<(ostream& os, Vector& v)
     {
         for(size_t i=0; i<v.size(); i++)
             os << v[i] << ' ';

         return os << '\n';
     }

     public:
     Vector(int n=5) : sz(n), data( new int[n])
     {
         for(size_t i=0; i < sz; i++)
         data[i] = 0;
     }

     Vector(const Vector& r) :              // the copy constructor
         sz(r.size()),
         data( new int[sz] )
     {
         for(size_t i=0; i < sz; i++)
             data[i] = r.data[i];
     }


     Vector(const Vector&& r)               // the move constructor
             sz{r.size()},
             data{r.data}
     { }

     Vector& operator= (const Vector& r)    // the copy assignment
     {
         delete [] data;

         sz = r.size();
         data = new int[sz];

         for(size_t i=0; i < sz; i++)
             data[i] =  r.data[i];

         return *this;
     }

     Vector& operator= (const Vector&& r)    // the move assignment
     {
         delete [] data;

         data = new int[r.size()];
         data = r.data;

         return *this;
     }

     ~Vector()                                 // the destructor
     {
         cout << "deleting memory of array ";
         cout << *this;

         delete [] data;
         data = nullptr;
     }

     unsigned int size() const { return sz; }

     int& operator[] (int i) { return data[i]; }

     Vector operator+(Vector& rhs)
     {
         size_t maxdim = sz, mindim = sz;

         if(sz < rhs.size())
         {
             maxdim = rhs.size();
         }
         else
         {
             mindim = rhs.size();
         }

         Vector temp (maxdim);

         for(size_t i=0; i < mindim; i++)
             temp[i] = data[i] + rhs[i];

         return temp;
     }

     private:
     size_t  sz;
     int    *data;
};

int main()
{
     Vector v1(3), v2;

     cout << "v1.size() = " << v1.size() << '\n'
          << "v2.size() = " << v2.size() << "\n\n";

     v1[1] = 1; v2[2] = 3;   // using the index of '[]' operator

     cout << "v1 = " << v1;
     cout << "v2 = " << v2;


     Vector s;
     s = v1 + v2;         // these two lines give runtime error

//    Vector s = v1 + v2;     // this line is ok instead

     cout << "v1 + v2 = " << s << '\n';

     cout << "destroying objects..." << '\n';

     return 0;
}
// code ends here


Now I give a description of the error.
g++ executing this code on my gives:

v1.size() = 3
v2.size() = 5

v1 = 0 1 0
v2 = 0 0 3 0 0
deleting memory of array 0 1 3 0 0
v1 + v2 = 0 0 -1704935408 22058 0

destroying objects...
deleting memory of array 0 0 -1704935408 22058 0
free(): double free detected in tcache 2
aborted

As the title of the message says, I'm stuck.
I did my best to write it, but evidently some subtle error lays
between those lines.

If you comment the following:

     Vector s;
     s = v1 + v2;

and uncomment

//    Vector s = v1 + v2;

the program behaves correctly.
What could it be the problem?

Thank you.

[toc] | [next] | [standalone]

#70247

On 2019-12-04 17:54, alexo wrote:
> Hello guys,
> I've written this code to get acquainted to assignment operators,
> move constructors and move assignment operators, but I got a
> strange behavior in my code:
> I report it in its fullness because it's 123 lines:
> 
> // code starts here
> #include <iostream>
> 
> using std::cout;
> using std::ostream;
> 
> class Vector
> {
>      friend ostream& operator<<(ostream& os, Vector& v)
>      {
>          for(size_t i=0; i<v.size(); i++)
>              os << v[i] << ' ';
> 
>          return os << '\n';
>      }
> 
>      public:
>      Vector(int n=5) : sz(n), data( new int[n])
>      {
>          for(size_t i=0; i < sz; i++)
>          data[i] = 0;
>      }
> 
>      Vector(const Vector& r) :              // the copy constructor
>          sz(r.size()),
>          data( new int[sz] )
>      {
>          for(size_t i=0; i < sz; i++)
>              data[i] = r.data[i];
>      }
> 
> 
>      Vector(const Vector&& r)               // the move constructor
>              sz{r.size()},
>              data{r.data}
>      { }
> 
>      Vector& operator= (const Vector& r)    // the copy assignment
>      {
>          delete [] data;
> 
>          sz = r.size();
>          data = new int[sz];
> 
>          for(size_t i=0; i < sz; i++)
>              data[i] =  r.data[i];
> 
>          return *this;
>      }
> 
>      Vector& operator= (const Vector&& r)    // the move assignment
>      {
>          delete [] data;
> 
>          data = new int[r.size()];
>          data = r.data;
> 
>          return *this;
>      }
> 
>      ~Vector()                                 // the destructor
>      {
>          cout << "deleting memory of array ";
>          cout << *this;
> 
>          delete [] data;
>          data = nullptr;
>      }
> 
>      unsigned int size() const { return sz; }
> 
>      int& operator[] (int i) { return data[i]; }
> 
>      Vector operator+(Vector& rhs)
>      {
>          size_t maxdim = sz, mindim = sz;
> 
>          if(sz < rhs.size())
>          {
>              maxdim = rhs.size();
>          }
>          else
>          {
>              mindim = rhs.size();
>          }
> 
>          Vector temp (maxdim);
> 
>          for(size_t i=0; i < mindim; i++)
>              temp[i] = data[i] + rhs[i];
> 
>          return temp;
>      }
> 
>      private:
>      size_t  sz;
>      int    *data;
> };
> 
> int main()
> {
>      Vector v1(3), v2;
> 
>      cout << "v1.size() = " << v1.size() << '\n'
>           << "v2.size() = " << v2.size() << "\n\n";
> 
>      v1[1] = 1; v2[2] = 3;   // using the index of '[]' operator
> 
>      cout << "v1 = " << v1;
>      cout << "v2 = " << v2;
> 
> 
>      Vector s;
>      s = v1 + v2;         // these two lines give runtime error
> 
> //    Vector s = v1 + v2;     // this line is ok instead
> 
>      cout << "v1 + v2 = " << s << '\n';
> 
>      cout << "destroying objects..." << '\n';
> 
>      return 0;
> }
> // code ends here
> 
> 
> Now I give a description of the error.
> g++ executing this code on my gives:
> 
> v1.size() = 3
> v2.size() = 5
> 
> v1 = 0 1 0
> v2 = 0 0 3 0 0
> deleting memory of array 0 1 3 0 0
> v1 + v2 = 0 0 -1704935408 22058 0
> 
> destroying objects...
> deleting memory of array 0 0 -1704935408 22058 0
> free(): double free detected in tcache 2
> aborted
> 
> As the title of the message says, I'm stuck.
> I did my best to write it, but evidently some subtle error lays
> between those lines.
> 
> If you comment the following:
> 
>      Vector s;
>      s = v1 + v2;
> 
> and uncomment
> 
> //    Vector s = v1 + v2;
> 
> the program behaves correctly.
> What could it be the problem?
> 
> Thank you.

Hi

      Vector(Vector&& r) // no const
              sz{r.size()},
              data{r.data}
      {
          r.data = nullptr; // you move from it, so r is empty
          r.sz   = 0;
      }

Same in operator=(Vector&& r)

data{std::move(r.data)} would not help because it is a simple pointer.

[toc] | [prev] | [next] | [standalone]

#70248

On Wednesday, December 4, 2019 at 11:54:55 AM UTC-5, alexo wrote:
> 
> // code starts here
> #include <iostream>
> 
> using std::cout;
> using std::ostream;
> 
> class Vector
> {
>      friend ostream& operator<<(ostream& os, Vector& v)
>      {
>          for(size_t i=0; i<v.size(); i++)
>              os << v[i] << ' ';
> 
>          return os << '\n';
>      }
> 
>      public:
>      Vector(int n=5) : sz(n), data( new int[n])
>      {
>          for(size_t i=0; i < sz; i++)
>          data[i] = 0;
>      }
> 
>      Vector(const Vector& r) :              // the copy constructor
>          sz(r.size()),
>          data( new int[sz] )
>      {
>          for(size_t i=0; i < sz; i++)
>              data[i] = r.data[i];
>      }
> 
> 
>      Vector(const Vector&& r)               // the move constructor
>              sz{r.size()},
>              data{r.data}
>      { }
> 
>      Vector& operator= (const Vector& r)    // the copy assignment
>      {
>          delete [] data;
> 
>          sz = r.size();
>          data = new int[sz];
> 
>          for(size_t i=0; i < sz; i++)
>              data[i] =  r.data[i];
> 
>          return *this;
>      }
> 
>      Vector& operator= (const Vector&& r)    // the move assignment
>      {
>          delete [] data;
> 
>          data = new int[r.size()];
>          data = r.data;
> 
>          return *this;
>      }
> 
>      ~Vector()                                 // the destructor
>      {
>          cout << "deleting memory of array ";
>          cout << *this;
> 
>          delete [] data;
>          data = nullptr;
>      }
> 
>      unsigned int size() const { return sz; }
> 
>      int& operator[] (int i) { return data[i]; }
> 
>      Vector operator+(Vector& rhs)
>      {
>          size_t maxdim = sz, mindim = sz;
> 
>          if(sz < rhs.size())
>          {
>              maxdim = rhs.size();
>          }
>          else
>          {
>              mindim = rhs.size();
>          }
> 
>          Vector temp (maxdim);
> 
>          for(size_t i=0; i < mindim; i++)
>              temp[i] = data[i] + rhs[i];
> 
>          return temp;
>      }
> 
>      private:
>      size_t  sz;
>      int    *data;
> };
> 
> int main()
> {
>      Vector v1(3), v2;
> 
>      cout << "v1.size() = " << v1.size() << '\n'
>           << "v2.size() = " << v2.size() << "\n\n";
> 
>      v1[1] = 1; v2[2] = 3;   // using the index of '[]' operator
> 
>      cout << "v1 = " << v1;
>      cout << "v2 = " << v2;
> 
> 
>      Vector s;
>      s = v1 + v2;         // these two lines give runtime error
> 
> //    Vector s = v1 + v2;     // this line is ok instead
> 
>      cout << "v1 + v2 = " << s << '\n';
> 
>      cout << "destroying objects..." << '\n';
> 
>      return 0;
> }
> // code ends here
> 
> 
> What could it be the problem?
> 
One, your assignment operator defined, needs to check if this and &v are the same,

    Vector& operator=(const Vector& v)
    {
        if (this != &v)
        {
            // ...  
        }
        return *this;
    }

Two, your move constructor and move assignment operator are wrong, the parameter should be Vector&& r, not const Vector&& r, and the implementation should take r.data and replace r.data with nullptr, and take r.sz and replace r.sz with 0.

Three, size() should return size_t.

Four, you need a const version of operator[](int i)

Daniel

[toc] | [prev] | [next] | [standalone]

#70249

On Wednesday, December 4, 2019 at 1:01:15 PM UTC-5, Daniel wrote:
> On Wednesday, December 4, 2019 at 11:54:55 AM UTC-5, alexo wrote:
> > 
> > // code starts here
> > #include <iostream>
> > 
> > using std::cout;
> > using std::ostream;
> > 
> > class Vector
> > 
> >      Vector(const Vector&& r)               // the move constructor
> >              sz{r.size()},
> >              data{r.data}
> >      { }
> > 
> >      Vector& operator= (const Vector&& r)    // the move assignment
> >      {
> >          delete [] data;
> > 
> >          data = new int[r.size()];
> >          data = r.data;
> > 
> >          return *this;
> >      }

> Two, your move constructor and move assignment operator are wrong, the 
> parameter should be Vector&& r, not const Vector&& r, and the implementation 
> should take r.data and replace r.data with nullptr, and take r.sz and 
> replace r.sz with 0, for the move constructor, and swap data for r.data and > sz for r.sz, in the move assignment operator.

Daniel
> 

[toc] | [prev] | [next] | [standalone]

#70254

On 12/4/2019 5:54 PM, alexo wrote:
> Hello guys,
> I've written this code to get acquainted to assignment operators,
> move constructors and move assignment operators, but I got a
> strange behavior in my code:
> I report it in its fullness because it's 123 lines:
> 
> // code starts here
> #include <iostream>
> 
> using std::cout;
> using std::ostream;
> 
> class Vector
> {
>      friend ostream& operator<<(ostream& os, Vector& v)
>      {
>          for(size_t i=0; i<v.size(); i++)
>              os << v[i] << ' ';
> 
>          return os << '\n';
>      }
> 
>      public:
>      Vector(int n=5) : sz(n), data( new int[n])
>      {
>          for(size_t i=0; i < sz; i++)
>          data[i] = 0;
>      }
> 
>      Vector(const Vector& r) :              // the copy constructor
>          sz(r.size()),
>          data( new int[sz] )
>      {
>          for(size_t i=0; i < sz; i++)
>              data[i] = r.data[i];
>      }
> 
> 
>      // Vector(const Vector&& r)               // the move constructor
        Vector(Vector&& r) :              // the move constructor
>              sz{r.size()},
>              data{r.data}
>      {
	r.sz = 0;
	r.data = nullptr;

	}
> 
>      // Vector& operator= (const Vector& r)    // the copy assignment
        Vector& operator= (Vector& r)    // the copy assignment
>      {
>          delete [] data;
> 
>          sz = r.size();
>          data = new int[sz];
> 
>          for(size_t i=0; i < sz; i++)
>              data[i] =  r.data[i];
> 
>          return *this;
>      }
> 
>      Vector& operator= (const Vector&& r)    // the move assignment
>      {
>          delete [] data;
> 
	// loose the following
>          // data = new int[r.size()];
>          data = r.data;
	s.sz = r.sz;
	r.data = nullptr;
	r.sz = 0;

> 
>          return *this;
>      }
> 
>      ~Vector()                                 // the destructor
>      {
>          cout << "deleting memory of array ";
>          cout << *this;
> 
	if (data)
	{
>          delete [] data;
>          data = nullptr;
	}
>      }
> 
>      unsigned int size() const { return sz; }
> 
>      int& operator[] (int i) { return data[i]; }
> 
>      Vector operator+(Vector& rhs)
>      {
>          size_t maxdim = sz, mindim = sz;
> 
>          if(sz < rhs.size())
>          {
>              maxdim = rhs.size();
>          }
>          else
>          {
>              mindim = rhs.size();
>          }
> 
>          Vector temp (maxdim);
> 
>          for(size_t i=0; i < mindim; i++)
>              temp[i] = data[i] + rhs[i];
> 
>          return temp;
>      }
> 
>      private:
>      size_t  sz;
>      int    *data;
> };
> 
> int main()
> {
>      Vector v1(3), v2;
> 
>      cout << "v1.size() = " << v1.size() << '\n'
>           << "v2.size() = " << v2.size() << "\n\n";
> 
>      v1[1] = 1; v2[2] = 3;   // using the index of '[]' operator
> 
>      cout << "v1 = " << v1;
>      cout << "v2 = " << v2;
> 
> 
>      Vector s;
>      s = v1 + v2;         // these two lines give runtime error
> 
> //    Vector s = v1 + v2;     // this line is ok instead
> 
>      cout << "v1 + v2 = " << s << '\n';
> 
>      cout << "destroying objects..." << '\n';
> 
>      return 0;
> }
> // code ends here

The following is a good help:

  c++ -std=c++17 -Wall yetAnotherVector.cc && valgrind ./a.out


> 
> 
> Now I give a description of the error.
> g++ executing this code on my gives:
> 
> v1.size() = 3
> v2.size() = 5
> 
> v1 = 0 1 0
> v2 = 0 0 3 0 0
> deleting memory of array 0 1 3 0 0
> v1 + v2 = 0 0 -1704935408 22058 0
> 
> destroying objects...
> deleting memory of array 0 0 -1704935408 22058 0
> free(): double free detected in tcache 2
> aborted
> 
> As the title of the message says, I'm stuck.
> I did my best to write it, but evidently some subtle error lays
> between those lines.
> 
> If you comment the following:
> 
>      Vector s;
>      s = v1 + v2;
> 
> and uncomment
> 
> //    Vector s = v1 + v2;
> 
> the program behaves correctly.
> What could it be the problem?
> 
> Thank you.

[toc] | [prev] | [next] | [standalone]

#70264

On 12/4/19 8:16 PM, Manfred wrote:
>>

Sorry, wrong line. Itis the /move/ assignment operator that should get a 
non-const reference.

>>      // Vector& operator= (const Vector& r)    // the copy assignment
>         Vector& operator= (Vector& r)    // the copy assignment
>>      {
>>          delete [] data;
>>
>>          sz = r.size();
>>          data = new int[sz];
>>
>>          for(size_t i=0; i < sz; i++)
>>              data[i] =  r.data[i];
>>
>>          return *this;
>>      }
>>
>>      Vector& operator= (const Vector&& r)    // the move assignment

[toc] | [prev] | [next] | [standalone]

#70271

Il 05/12/19 01:42, Manfred ha scritto:
> On 12/4/19 8:16 PM, Manfred wrote:
>>>
> 
> Sorry, wrong line. Itis the /move/ assignment operator that should get a 
> non-const reference.
> 
>>>      // Vector& operator= (const Vector& r)    // the copy assignment
>>         Vector& operator= (Vector& r)    // the copy assignment
>>>      {
>>>          delete [] data;
>>>
>>>          sz = r.size();
>>>          data = new int[sz];
>>>
>>>          for(size_t i=0; i < sz; i++)
>>>              data[i] =  r.data[i];
>>>
>>>          return *this;
>>>      }
>>>
>>>      Vector& operator= (const Vector&& r)    // the move assignment
> 

So To summarize, if I have well understood, this should be the four 
operators:

       Vector(const Vector& r) :              // the copy constructor
           sz(r.size()),
           data( new int[sz] )
       {
           for(size_t i=0; i < sz; i++)
               data[i] = r.data[i];
       }

       Vector(Vector&& r) :                   // the move constructor
               sz{r.size()},
               data{r.data}
       {
           r.sz = 0;
           r.data = nullptr;
       }

       Vector& operator= (const Vector& r)    // the copy assignment
       {
           if(this != &r)
           {
               delete [] data;

               sz = r.size();
               data = new int[sz];

               for(size_t i=0; i < sz; i++)
                   data[i] =  r.data[i];
               }

           return *this;
       }

       Vector& operator= (const Vector&& r)    // the move assignment
       {
           if(data)
             delete [] data;

           data = r.data;

           return *this;
       }

Thank you.

[toc] | [prev] | [next] | [standalone]

#70272

Il 05/12/19 03:45, alexo ha scritto:
> Il 05/12/19 01:42, Manfred ha scritto:
>> On 12/4/19 8:16 PM, Manfred wrote:
>>>>
>>
>> Sorry, wrong line. Itis the /move/ assignment operator that should get 
>> a non-const reference.
>>


> So To summarize, if I have well understood, this should be the four 
> operators:
> 
>        Vector(const Vector& r) :              // the copy constructor
>            sz(r.size()),
>            data( new int[sz] )
>        {
>            for(size_t i=0; i < sz; i++)
>                data[i] = r.data[i];
>        }
> 
>        Vector(Vector&& r) :                   // the move constructor
>                sz{r.size()},
>                data{r.data}
>        {
>            r.sz = 0;
>            r.data = nullptr;
>        }
> 
>        Vector& operator= (const Vector& r)    // the copy assignment
>        {
>            if(this != &r)
>            {
>                delete [] data;
> 
>                sz = r.size();
>                data = new int[sz];
> 
>                for(size_t i=0; i < sz; i++)
>                    data[i] =  r.data[i];
>                }
> 
>            return *this;
>        }
> 
>        Vector& operator= (const Vector&& r)    // the move assignment
>        {
>            if(data)
>              delete [] data;
> 
>            data = r.data;
> 
>            return *this;
>        }
> 
> Thank you.

No, Manfred
That gives the runtime error again. Both the move compy constructor and 
the move assignment operator should take non const reference.

If I write them this way everything works:

       Vector(Vector&& r) :              // the move constructor
               sz{r.size()},
               data{r.data}
       {
           r.sz = 0;
           r.data = nullptr;
       }

       Vector& operator= (Vector&& r)    // the move assignment
       {
           if(data)
             delete [] data;

           data = r.data;
           r.data = nullptr;
           r.sz = 0;

           return *this;
       }

[toc] | [prev] | [next] | [standalone]

#70273

On 05/12/2019 02:58, alexo wrote:
> Il 05/12/19 03:45, alexo ha scritto:
>> Il 05/12/19 01:42, Manfred ha scritto:
>>> On 12/4/19 8:16 PM, Manfred wrote:
>>>>>
>>>
>>> Sorry, wrong line. Itis the /move/ assignment operator that should
>>> get a non-const reference.
>>>
>
>
>> So To summarize, if I have well understood, this should be the four
>> operators:
>>
>>        Vector(const Vector& r) :              // the copy constructor
>>            sz(r.size()),
>>            data( new int[sz] )
>>        {
>>            for(size_t i=0; i < sz; i++)
>>                data[i] = r.data[i];
>>        }
>>
>>        Vector(Vector&& r) :                   // the move constructor
>>                sz{r.size()},
>>                data{r.data}
>>        {
>>            r.sz = 0;
>>            r.data = nullptr;
>>        }
>>
>>        Vector& operator= (const Vector& r)    // the copy assignment
>>        {
>>            if(this != &r)
>>            {
>>                delete [] data;
>>
>>                sz = r.size();
>>                data = new int[sz];
>>
>>                for(size_t i=0; i < sz; i++)
>>                    data[i] =  r.data[i];
>>                }
>>
>>            return *this;
>>        }
>>
>>        Vector& operator= (const Vector&& r)    // the move assignment
>>        {
>>            if(data)
>>              delete [] data;
>>
>>            data = r.data;
>>
>>            return *this;
>>        }
>>
>> Thank you.
>
> No, Manfred
> That gives the runtime error again. Both the move compy constructor and
> the move assignment operator should take non const reference.
>
> If I write them this way everything works:
>
>       Vector(Vector&& r) :              // the move constructor
>               sz{r.size()},
>               data{r.data}
>       {
>           r.sz = 0;
>           r.data = nullptr;
>       }
>
>       Vector& operator= (Vector&& r)    // the move assignment
>       {
>           if(data)
>             delete [] data;
>
>           data = r.data;
>           r.data = nullptr;
>           r.sz = 0;
>
>           return *this;
>       }

Yes, both the Vector&& r  should be non-const.  That's because having 
"stolen" the resources from r, you need to update r to reflect this, 
otherwise the memory for the data member will be referenced in two 
distinct objects, and so when the objects are destroyed both of them 
will attempt to free the memory, giving the errors you see.

Also there's another error above:  in the move-assignment function you 
steal the data resource from r, but you also need to set sz to correctly 
reflect the size of the array you've stolen.

I.e. you need both these lines:

        data = r.data;
        sz = r.sz;

(not just the data line).  This didn't show up as a problem in your 
output, because it just so happens sz has the default constructor 
setting of 5, and r.sz also happened to be 5 (but in other programs 
could be different).

Regards,
Mike.

[toc] | [prev] | [next] | [standalone]

#70276

Il 05/12/19 05:10, Mike Terry ha scritto:
> On 05/12/2019 02:58, alexo wrote:
>> Il 05/12/19 03:45, alexo ha scritto:
>>> Il 05/12/19 01:42, Manfred ha scritto:
>>>> On 12/4/19 8:16 PM, Manfred wrote:
>>>>>>
>>>>
>>>> Sorry, wrong line. Itis the /move/ assignment operator that should
>>>> get a non-const reference.
>>>>
>>
>>
>>> So To summarize, if I have well understood, this should be the four
>>> operators:
>>>
>>>        Vector(const Vector& r) :              // the copy constructor
>>>            sz(r.size()),
>>>            data( new int[sz] )
>>>        {
>>>            for(size_t i=0; i < sz; i++)
>>>                data[i] = r.data[i];
>>>        }
>>>
>>>        Vector(Vector&& r) :                   // the move constructor
>>>                sz{r.size()},
>>>                data{r.data}
>>>        {
>>>            r.sz = 0;
>>>            r.data = nullptr;
>>>        }
>>>
>>>        Vector& operator= (const Vector& r)    // the copy assignment
>>>        {
>>>            if(this != &r)
>>>            {
>>>                delete [] data;
>>>
>>>                sz = r.size();
>>>                data = new int[sz];
>>>
>>>                for(size_t i=0; i < sz; i++)
>>>                    data[i] =  r.data[i];
>>>                }
>>>
>>>            return *this;
>>>        }
>>>
>>>        Vector& operator= (const Vector&& r)    // the move assignment
>>>        {
>>>            if(data)
>>>              delete [] data;
>>>
>>>            data = r.data;
>>>
>>>            return *this;
>>>        }
>>>
>>> Thank you.
>>
>> No, Manfred
>> That gives the runtime error again. Both the move compy constructor and
>> the move assignment operator should take non const reference.
>>
>> If I write them this way everything works:
>>
>>       Vector(Vector&& r) :              // the move constructor
>>               sz{r.size()},
>>               data{r.data}
>>       {
>>           r.sz = 0;
>>           r.data = nullptr;
>>       }
>>
>>       Vector& operator= (Vector&& r)    // the move assignment
>>       {
>>           if(data)
>>             delete [] data;
>>
>>           data = r.data;
>>           r.data = nullptr;
>>           r.sz = 0;
>>
>>           return *this;
>>       }
> 
> Yes, both the Vector&& r  should be non-const.  That's because having 
> "stolen" the resources from r, you need to update r to reflect this, 
> otherwise the memory for the data member will be referenced in two 
> distinct objects, and so when the objects are destroyed both of them 
> will attempt to free the memory, giving the errors you see.
> 
> Also there's another error above:  in the move-assignment function you 
> steal the data resource from r, but you also need to set sz to correctly 
> reflect the size of the array you've stolen.
> 
> I.e. you need both these lines:
> 
>         data = r.data;
>         sz = r.sz;
> 
> (not just the data line).  This didn't show up as a problem in your 
> output, because it just so happens sz has the default constructor 
> setting of 5, and r.sz also happened to be 5 (but in other programs 
> could be different).
> 
> Regards,
> Mike.
> 
> 

Hi Mike,
thank you for your explanation of the error. I didn't understand why 
there were a double free call.

thank you

[toc] | [prev] | [next] | [standalone]

#70287

On 12/5/2019 3:58 AM, alexo wrote:
> No, Manfred
> That gives the runtime error again. Both the move compy constructor and 
> the move assignment operator should take non const reference.

Yes, that's what I wrote in my second message.

> 
> 
>        Vector& operator= (Vector&& r)    // the move assignment
>        {
>            if(data)
>              delete [] data;
> 
	
here sz should be set as well (as in my first post, where s.sz was 
another typo, though)

>            data = r.data;
>            r.data = nullptr;
>            r.sz = 0;
> 
>            return *this;
>        }

[toc] | [prev] | [next] | [standalone]

#70260

Il 04/12/19 17:54, alexo ha scritto:
> Hello guys,
> I've written this code to get acquainted to assignment operators,
> move constructors and move assignment operators, but I got a
> strange behavior in my code:

I post one answer to all of you that replied to my request:

@Manfred:

I didn't know what Valgrind was. Your last line showed me how to
invoke it. If I have well understood it is a memory analyzer tool.

@Volker Wippel:

You are right, I didn't think about that:
in the copy assignment operator the data copy should be only
performed if the righ hand side of the = operator is not the same
as the left hand side.

@Daniel

your point 4)

I added

int& operator[] (int i) const { return data[i]; }

to my code, but I had to modify operator<< as well:

friend ostream& operator<<(ostream& os, const Vector& v)

I thank you all for your prompt reply.

[toc] | [prev] | [next] | [standalone]

#70265

On Wednesday, December 4, 2019 at 4:52:46 PM UTC-5, alexo wrote:
> 
> @Daniel
> 
> your point 4)
> 
> I added
> 
> int& operator[] (int i) const { return data[i]; }
> 
Should be

int operator[] (int i) const { return data[i]; }

Daniel

[toc] | [prev] | [next] | [standalone]

#70270

Il 05/12/19 01:47, Daniel ha scritto:
> On Wednesday, December 4, 2019 at 4:52:46 PM UTC-5, alexo wrote:
>>
>> @Daniel
>>
>> your point 4)
>>
>> I added
>>
>> int& operator[] (int i) const { return data[i]; }
>>
> Should be
> 
> int operator[] (int i) const { return data[i]; }
> 
> Daniel
> 
You are right, I'm learning from my mistakes.
Thank you

[toc] | [prev] | [standalone]

Back to top | Article view | comp.lang.c++

csiph-web