simple delete question

Discussion in 'C++' started by cppaddict, May 20, 2004.

  1. cppaddict

    cppaddict Guest

    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;
    }
    cppaddict, May 20, 2004
    #1
    1. Advertising

  2. "cppaddict" <> wrote in message
    news:...
    > 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
    John Harrison, May 20, 2004
    #2
    1. Advertising

  3. cppaddict wrote:

    > 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
    =?UTF-8?B?IkRhcmlvIChkcmlua2luZyBjb++sgGVlIGluIHRo, May 20, 2004
    #3
  4. "John Harrison" <> wrote in message
    news:...
    >
    > "cppaddict" <> wrote in message
    > news:...
    > > 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.
    >


    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, May 20, 2004
    #4
  5. "Dario (drinking coï¬?ee in the oï¬fceâ?¦)" <> wrote in
    message news:c8i6nt$ahr$...
    > cppaddict wrote:
    >
    > > 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;
    > }
    >


    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
    John Harrison, May 20, 2004
    #5
  6. John Harrison wrote:

    > "Dario (drinking coï¬?ee in the oï¬fceâ?¦)" <> wrote in
    > message news:c8i6nt$ahr$...
    >
    >> cppaddict wrote:
    >>
    >>> 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;
    >> }

    >
    > 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
    =?UTF-8?B?IkRhcmlvIChkcmlua2luZyBjb++sgGVlIGluIHRo, May 20, 2004
    #6
  7. "Dario (drinking co?ee in the o?ce.)" <> wrote in message
    news:c8i7td$ddi$...
    > John Harrison wrote:
    >
    > > "Dario (drinking coï¬?ee in the oï¬fceâ?¦)" <> wrote

    in
    > > message news:c8i6nt$ahr$...
    > >
    > >> cppaddict wrote:
    > >>
    > >>> 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;
    > >> }

    > >
    > > That works but why?? Needless use of new, inefficient and error prone.
    > >
    > > _pixelPoints.push_back(Point(x,y));

    >
    > Good point.


    Ho ho!

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


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

    john
    John Harrison, May 20, 2004
    #7
  8. cppaddict

    Jeff Flinn Guest

    "John Harrison" <> wrote in message
    news:...
    >
    > "Dario (drinking co?ee in the o?ce.)" <> wrote in

    message
    > news:c8i7td$ddi$...
    > > John Harrison wrote:
    > >
    > > > "Dario (drinking coï¬?ee in the oï¬fceâ?¦)" <>

    wrote
    [snip]
    > >
    > > > 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)
    > >

    >
    > 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
    Jeff Flinn, May 20, 2004
    #8
  9. cppaddict

    Pete Becker Guest

    John Harrison wrote:
    >
    > 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.

    --

    Pete Becker
    Dinkumware, Ltd. (http://www.dinkumware.com)
    Pete Becker, May 20, 2004
    #9
  10. 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

    In <> cppaddict wrote:
    > 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;
    >}
    >
    >
    >
    Stefan Pantos, May 20, 2004
    #10
  11. "Stefan Pantos" <> wrote in message
    news:...
    > 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
    John Harrison, May 20, 2004
    #11
  12. cppaddict

    cppaddict Guest

    >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
    cppaddict, May 20, 2004
    #12
  13. cppaddict

    Howard Guest

    >
    > 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
    Howard, May 20, 2004
    #13
  14. "Howard" <> wrote in message
    news:HQ5rc.8675$...
    > >
    > > 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?)
    >


    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, May 20, 2004
    #14
  15. "cppaddict" <> wrote in message
    news:...
    > >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.


    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

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

    >
    > 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
    John Harrison, May 20, 2004
    #15
  16. cppaddict

    cppaddict Guest

    >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
    cppaddict, May 20, 2004
    #16
  17. "cppaddict" <> wrote in message
    news:...
    > >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?


    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
    John Harrison, May 20, 2004
    #17
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Sandeep Grover

    delete on delete !

    Sandeep Grover, Jul 12, 2003, in forum: C++
    Replies:
    19
    Views:
    608
    Chris \( Val \)
    Jul 22, 2003
  2. HeroOfSpielburg
    Replies:
    1
    Views:
    376
    Alf P. Steinbach
    Aug 6, 2003
  3. 0to60
    Replies:
    4
    Views:
    305
    Jerry Coffin
    Dec 19, 2003
  4. Mathieu Malaterre

    delete NULL, what about delete[] NULL

    Mathieu Malaterre, Aug 17, 2004, in forum: C++
    Replies:
    2
    Views:
    3,807
    Mathieu Malaterre
    Aug 17, 2004
  5. DamonChong
    Replies:
    11
    Views:
    547
    msalters
    Jan 31, 2005
Loading...

Share This Page