simple delete question

[cppaddict]

Hi,

I am deleting some objects created by new in my class destructor, and
it is causing my application to error at runtime. The code below
compiles ok, and also runs fine if I remove the body of the
destructor. So I think I am somehow using delete incorrectly, but I'm
not sure exaclty what I'm doing wrong.

I'd apprecitate any clarification and suggestions for rewriting the
below properly.

Thanks,
cpp

PS: The Char class below is used to store a pixel representation of a
character for display. A character in this context is simply a
collection of points.

#include <vector>
#include "Point.h"

class Char {
private:
char _ch;
std::vector<Point> _pixelPoints;
public:
Char(char);
~Char();
void addPoint(int, int);
std::vector<Point>& const getPixelPoints();
char getChar() {return _ch;}
};

Char::Char(char ch) :
_ch(ch)
{ }

Char::~Char() {
std::vector<Point>::iterator iter;
for (iter=_pixelPoints.begin(); iter!=_pixelPoints.end();
iter++)
delete iter;
}

void Char::addPoint(int x, int y) {
_pixelPoints.push_back(*( new Point(x,y) ));
}

std::vector<Point>& const Char::getPixelPoints() {
return _pixelPoints;
}

 

[John Harrison]

Hi,

I am deleting some objects created by new in my class destructor, and
it is causing my application to error at runtime. The code below
compiles ok, and also runs fine if I remove the body of the
destructor. So I think I am somehow using delete incorrectly, but I'm
not sure exaclty what I'm doing wrong.

What you are doing wrong is using new and delete at all. There seems to be
an epidemic of needless use of new on this group at the moment.

Corrections below.

I'd apprecitate any clarification and suggestions for rewriting the
below properly.

Thanks,
cpp

PS: The Char class below is used to store a pixel representation of a
character for display. A character in this context is simply a
collection of points.

#include <vector>
#include "Point.h"

class Char {
private:
char _ch;
std::vector<Point> _pixelPoints;
public:
Char(char);
~Char();
void addPoint(int, int);
std::vector<Point>& const getPixelPoints();
char getChar() {return _ch;}
};

Char::Char(char ch) :
_ch(ch)
{ }

Char::~Char() {
std::vector<Point>::iterator iter;
for (iter=_pixelPoints.begin(); iter!=_pixelPoints.end();
iter++)
delete iter;
}

Don't need the destructor, remove it.

void Char::addPoint(int x, int y) {
_pixelPoints.push_back(*( new Point(x,y) ));

Don't need to use new

_pixelPoints.push_back(Point(x,y));

}

std::vector<Point>& const Char::getPixelPoints() {
return _pixelPoints;
}

Should be fine now.

john

 

[Guest]

I am deleting some objects created by new in my class destructor, and
it is causing my application to error at runtime. The code below
compiles ok, and also runs fine if I remove the body of the
destructor. So I think I am somehow using delete incorrectly, but I'm
not sure exaclty what I'm doing wrong.

A question: Why you "delete" something that you do not "new" ?

In addPoint you add:
*( new Point(x,y) )
and not:
new Point(x,y)
so you cannot delete it.

Probably you can rewrite your addPoint as:

void Char::addPoint(int x, int y) {
Point * p = new Point(x,y);
_pixelPoints.push_back(*p));
delete p;
}

and avoid to delete anything in ~Char().

Just my 2 cents...

- Dario

 

[John Harrison]

What you are doing wrong is using new and delete at all. There seems to be
an epidemic of needless use of new on this group at the moment.

I'm curious as to what led you to use new at all. After all there are no
pointers in your code, so why did you consider using new?

Seems to be happening a lot lately so I'm interested in what makes new and
delete (and pointers generally) so attractive to newbies. Maybe you can
explain.

john

 

[John Harrison]

A question: Why you "delete" something that you do not "new" ?

In addPoint you add:
*( new Point(x,y) )
and not:
new Point(x,y)
so you cannot delete it.

Probably you can rewrite your addPoint as:

void Char::addPoint(int x, int y) {
Point * p = new Point(x,y);
_pixelPoints.push_back(*p));
delete p;
}

That works but why?? Needless use of new, inefficient and error prone.

_pixelPoints.push_back(Point(x,y));

What's so hard about the above that newbies fail to use it?

john

 

[Guest]

That works but why?? Needless use of new, inefficient and error prone.

_pixelPoints.push_back(Point(x,y));

Good point.

What's so hard about the above that newbies fail to use it?

The explanation (at least for me) is the fact that I'm used
(since many years) to write in Java, where the only way to create
a new Object is via the "new" operator.
So (at least for me) the natural way is always
new Point(x, y)

But definitively in C++ the example mu be written as you said.

- Dario

 

[John Harrison]

Good point.

Ho ho!

The explanation (at least for me) is the fact that I'm used
(since many years) to write in Java, where the only way to create
a new Object is via the "new" operator.
So (at least for me) the natural way is always
new Point(x, y)

Yes that explains some of it. But I definitely get the impression that
non-Java programmers also make similar mistakes.

john

 

[Jeff Flinn]

wrote
[snip]

The explanation (at least for me) is the fact that I'm used
(since many years) to write in Java, where the only way to create
a new Object is via the "new" operator.
So (at least for me) the natural way is always
new Point(x, y)

Yes that explains some of it. But I definitely get the impression that
non-Java programmers also make similar mistakes.

When I joined my current employer 2 years ago, our product was rife with
unnecessary new/delete usage. My predecessor along with most of the other
employees are long time C programmers. At least they did use new/delete
rather than malloc/free.

Jeff F

 

[Pete Becker]

Seems to be happening a lot lately so I'm interested in what makes new and
delete (and pointers generally) so attractive to newbies. Maybe you can
explain.

Java pollution.

 

[Stefan Pantos]

I'm not sure that you understand what happens when you add something to
a vector. The vector contructs a copy of the object you pass it and
stores this copy. This is why you do not need to delete what you have
added.

This might seem like it will be slow and well it could be. To get around
this you could store a vector of pointers vector<Point*> now it will
make a copy of your pointer not the object. Now you would have to delete
what you have added to your vector.

In java you could view all you objects as pointers and that is way you
need a new and also why you can assing them the value nil.

Stefan

 

[John Harrison]

I'm not sure that you understand what happens when you add something to
a vector. The vector contructs a copy of the object you pass it and
stores this copy. This is why you do not need to delete what you have
added.

This might seem like it will be slow and well it could be. To get around
this you could store a vector of pointers vector<Point*> now it will
make a copy of your pointer not the object. Now you would have to delete
what you have added to your vector.

Seems unlikely in this case. Presumably Point is some small class (two ints
perhaps). Dynamically allocating a large number of small objects is a
notorious inefficiency. By constrast copying small objects is going to be
much more efficient.

If you had a very large object, then the cost of copying might exceed the
cost of allocating it, but even in that case you would normally use a smart
pointer rather than a raw pointer.

john

 

[cppaddict]

I'm curious as to what led you to use new at all. After all there are no

pointers in your code, so why did you consider using new?

Seems to be happening a lot lately so I'm interested in what makes new and
delete (and pointers generally) so attractive to newbies. Maybe you can
explain.

Thank you all for the clarification.

John,

To answer your question, my erroneous thought process was simply this:
Since the number of Points in a Char object varies, that means the
memory must be dynamically allocated at runtime, and therefore that
means I will have to use the new operator. Or, to break it down:

Don't know how many Points are in a Char //step 1 in my reasoning
implies==>dynamic allocation //step 2
imples==> use new operator //step 3

Correct me if I'm wrong, but looking back at the code now it seems to
me that my logical error was in step 2.

There was an additional consideration too, which someone else
mentioned, and which you can't see from the posted code, which I
simplified in order to give focus to my question. That additional
consideraton was my belief that the cost of copying an object (even a
small one) was more than the cost of dynamic allocation -- indeed, I
was not aware that dynamic allocation had a high cost at all.

Originally, the signature of addPoint() looked like this:

void addPoint(Point& const);

Thus I believed that I was efficiently creating a new Point object, as
well as avoiding any overhead involved in copying an object (since I
was passing by reference). Please let me know if there are additional
logical erros in THAT statement.

Finally, you mention that even if the Point object was large, it would
be better to use smart pointers rather than to store a vector of
pointers which you delete yourself. Why is this so? Wouldn't the
cost of those things be the same?

Thanks again,
cpp

 

[Howard]

That works but why?? Needless use of new, inefficient and error prone.

_pixelPoints.push_back(Point(x,y));

What's so hard about the above that newbies fail to use it?

I think it might be that this looks like a direct call to a class'
constructor, which they are told is not allowed. In reality it's the
construction of a temporary object, and that object is then copied by the
push_back function, and the temporary destroyed. (Although I think the
compiler is allowed to omit the temporary I think, and directly initialize
the copy used by push_back...right?)

In any case, new C++ coders are taught, in general, that there are only two
ways to construct an object. One is using a simple "static" declaration, as
in

Point p;

or,

Point p(4,5);

and the other is to dynamically allocate it, as in

Point* p;
....
p = new Point(4,5);

or

Point p = new Point(4,5);

I don't recall ever seeing anyone do it like this

foo(Point(4,5));

until I'd been watching this newsgroup for a while. It doesn't seem to fit
with either of the "two methods" we we're originally taught, and you have to
admit it's a little obscure until you've actually used it yourself. It's
not that it's hard to grasp...just that nobody tells them it's legal (and
useful)!

Maybe we simply need to teach newbies this as a "third" method of
constructing an object?

-Howard

 

[John Harrison]

I think it might be that this looks like a direct call to a class'
constructor, which they are told is not allowed. In reality it's the
construction of a temporary object, and that object is then copied by the
push_back function, and the temporary destroyed. (Although I think the
compiler is allowed to omit the temporary I think, and directly initialize
the copy used by push_back...right?)

Maybe but it doesn't explain why new is used. The following avoids using any
temporaries.

Point tmp(x, y);
_pixelPoints.push_back(tmp);

Maybe it's a lack of confidence with C++ value semantics. A newbie might
worry that tmp is going to go out of scope and when that happens the value
on the vector will be invalidated in some way.

john

 

[John Harrison]

Thank you all for the clarification.

John,

To answer your question, my erroneous thought process was simply this:
Since the number of Points in a Char object varies, that means the
memory must be dynamically allocated at runtime, and therefore that
means I will have to use the new operator. Or, to break it down:

Don't know how many Points are in a Char //step 1 in my reasoning
implies==>dynamic allocation //step 2
imples==> use new operator //step 3

Correct me if I'm wrong, but looking back at the code now it seems to
me that my logical error was in step 2.

I think the conceptual error is that you are confusing the container with
the contained objects. It is true that the container (i.e. the vector) has
to use new (or something like it) in order hold a varying number of Points.
But that use of new is in the vector code which has already been written
for you, it not necessary to use it in your code.

Consider a vector of integers, would you have tried to allocate each integer
using new, like this?

vector<int> x;
x.push_back(*(new int(5));

Or would you have just written

There was an additional consideration too, which someone else
mentioned, and which you can't see from the posted code, which I
simplified in order to give focus to my question. That additional
consideraton was my belief that the cost of copying an object (even a
small one) was more than the cost of dynamic allocation -- indeed, I
was not aware that dynamic allocation had a high cost at all.

No, definitely not, the cost of allocating large numbers of small objects is
going to greatly exceed the cost of copying those objects.

Originally, the signature of addPoint() looked like this:

void addPoint(Point& const);

Thus I believed that I was efficiently creating a new Point object, as
well as avoiding any overhead involved in copying an object (since I
was passing by reference). Please let me know if there are additional
logical erros in THAT statement.

Right, so you were coding like this?

Char c;
c.addPoint(*(new Point(x, y)));

You are right that in general using a const reference is a good idea to
avoid unecessary copies. But again the same change should be made

Char c;
c.addPoint(Point(x, y));

Finally, you mention that even if the Point object was large, it would
be better to use smart pointers rather than to store a vector of
pointers which you delete yourself. Why is this so? Wouldn't the
cost of those things be the same?

Similar, in fact the smart pointer will be fractionally more expensive. The
point of smart pointers is that they are much less error prone. Because a
smart pointer takes care of the burden of deleteing an object for you its
impossible to forget to delete a pointer (thus causing a memory leak) or
delete a pointer twice (probably crashing your program).

john

 

[cppaddict]

No, definitely not, the cost of allocating large numbers of small objects is

going to greatly exceed the cost of copying those objects.

Good to know.

You are right that in general using a const reference is a good idea to
avoid unecessary copies. But again the same change should be made

Char c;
c.addPoint(Point(x, y));

Yet the compiler won't let me create a vector of Point references.
That is, the following is not allowed:

std::vector<Point&> vectorOfPointReferences;

So it seems there is no way to avoid making copies of Point when you
add it to the vector. That is, the Point argument to push_back will
always have to be created and then copied to the vector. Is there any
way to avoid this unnecessary copying, even if is less burdensome than
unnessary allocating. It seems that there should be. Am I still
missing something?

Similar, in fact the smart pointer will be fractionally more expensive. The
point of smart pointers is that they are much less error prone. Because a
smart pointer takes care of the burden of deleteing an object for you its
impossible to forget to delete a pointer (thus causing a memory leak) or
delete a pointer twice (probably crashing your program).

Good point. I will keep it in mind.

Thanks again,
cpp

 

[John Harrison]

Good to know.



Yet the compiler won't let me create a vector of Point references.
That is, the following is not allowed:

std::vector<Point&> vectorOfPointReferences;

So it seems there is no way to avoid making copies of Point when you
add it to the vector. That is, the Point argument to push_back will
always have to be created and then copied to the vector. Is there any
way to avoid this unnecessary copying, even if is less burdensome than
unnessary allocating. It seems that there should be. Am I still
missing something?

Its part of the design of STL containers that they hold copies of the
objects that are added to them. Anything else would be virtually unusable.

vector<string> vec;
for (...)
{
string s = "123";
vec.push_back(s);
s = "abc"; // s modified here, should that also affect vec?
} // s destroyed here, should that affect vec?

Fortunately the answer to both questions is no. And that is only possible
because vec has taken a copy of the original string s.

john