Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss
I'm not able to find the error
63 views
Skip to first unread message
alexo
Dec 4, 2019, 11:54:55 AM12/4/19
to
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.
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.
Volker Wippel
Dec 4, 2019, 12:41:07 PM12/4/19
to
Vector(Vector&& r) // no const
sz{r.size()},
data{r.data}
{
r.data = nullptr; // you move from it, so r is empty
data{r.data}
{
r.sz = 0;
}
Same in operator=(Vector&& r)
data{std::move(r.data)} would not help because it is a simple pointer.
Daniel
Dec 4, 2019, 1:01:15 PM12/4/19
to
> 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
Daniel
Dec 4, 2019, 1:06:22 PM12/4/19
to
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
> >
> 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}
> > { }
> >
> > 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;
> > }
> > {
> > 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.
> 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
Daniel
>
Manfred
Dec 4, 2019, 2:17:11 PM12/4/19
to
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
> 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(Vector&& r) : // the move constructor
> sz{r.size()},
> data{r.data}
> {
r.sz = 0;
> data{r.data}
> {
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
> 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;
s.sz = r.sz;
r.data = nullptr;
r.sz = 0;
>
> return *this;
> }
>
> ~Vector() // the destructor
> {
> cout << "deleting memory of array ";
> cout << *this;
>
{
c++ -std=c++17 -Wall yetAnotherVector.cc && valgrind ./a.out
alexo
Dec 4, 2019, 4:52:46 PM12/4/19
to
Il 04/12/19 17:54, alexo ha scritto:
@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.
> 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:
> 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:
@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.
Manfred
Dec 4, 2019, 7:42:49 PM12/4/19
to
On 12/4/19 8:16 PM, Manfred wrote:
>>
Sorry, wrong line. Itis the /move/ assignment operator that should get a
non-const reference.
>>
Sorry, wrong line. Itis the /move/ assignment operator that should get a
non-const reference.
Daniel
Dec 4, 2019, 7:47:46 PM12/4/19
to
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
>
> @Daniel
>
> your point 4)
>
> I added
>
> int& operator[] (int i) const { return data[i]; }
>
int operator[] (int i) const { return data[i]; }
Daniel
alexo
Dec 4, 2019, 9:37:26 PM12/4/19
to
Il 05/12/19 01:47, Daniel ha scritto:
You are right, I'm learning from my mistakes.
Thank you
Thank you
alexo
Dec 4, 2019, 9:45:53 PM12/4/19
to
Il 05/12/19 01:42, Manfred ha scritto:
So To summarize, if I have well understood, this should be the four
operators:
Vector(const Vector& r) : // the copy constructor
if(this != &r)
if(data)
delete [] data;
data = r.data;
return *this;
}
Thank you.
operators:
Vector(const Vector& r) : // the copy constructor
sz(r.size()),
data( new int[sz] )
{
data( new int[sz] )
{
for(size_t i=0; i < sz; i++)
data[i] = r.data[i];
}
data[i] = r.data[i];
}
Vector(Vector&& r) : // the move constructor
sz{r.size()},
data{r.data}
{
r.sz = 0;
r.data = nullptr;
}
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
{
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;
}
alexo
Dec 4, 2019, 9:59:11 PM12/4/19
to
Il 05/12/19 03:45, alexo ha scritto:
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
> 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.
>>
>> On 12/4/19 8:16 PM, Manfred wrote:
>>>>
>>
>> Sorry, wrong line. Itis the /move/ assignment operator that should get
>> a non-const reference.
>>
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;
}
{
if(data)
delete [] data;
data = r.data;
if(data)
delete [] data;
data = r.data;
Mike Terry
Dec 4, 2019, 11:10:35 PM12/4/19
to
"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.
alexo
Dec 4, 2019, 11:45:23 PM12/4/19
to
Il 05/12/19 05:10, Mike Terry ha scritto:
Hi Mike,
thank you for your explanation of the error. I didn't understand why
there were a double free call.
thank you
thank you for your explanation of the error. I didn't understand why
there were a double free call.
thank you
Manfred
Dec 5, 2019, 11:29:32 AM12/5/19
to
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.
> No, Manfred
> That gives the runtime error again. Both the move compy constructor and
> the move assignment operator should take non const reference.
>
>
> Vector& operator= (Vector&& r) // the move assignment
> {
> if(data)
> delete [] data;
>
another typo, though)
0 new messages