simple delete question

Collapse
This topic is closed.
X
X
 
  • Time
  • Show
Clear All
new posts
  • cppaddict

    simple delete question

    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<Poi nt> _pixelPoints;
    public:
    Char(char);
    ~Char();
    void addPoint(int, int);
    std::vector<Poi nt>& const getPixelPoints( );
    char getChar() {return _ch;}
    };

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

    Char::~Char() {
    std::vector<Poi nt>::iterator iter;
    for (iter=_pixelPoi nts.begin(); iter!=_pixelPoi nts.end();
    iter++)
    delete iter;
    }

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

    std::vector<Poi nt>& const Char::getPixelP oints() {
    return _pixelPoints;
    }


  • John Harrison

    #2
    Re: simple delete question


    "cppaddict" <hello@hello.co m> wrote in message
    news:n46pa05qdj 8psnifg0t79jbop 20jledl89@4ax.c om...[color=blue]
    > 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.[/color]

    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.
    [color=blue]
    >
    > 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<Poi nt> _pixelPoints;
    > public:
    > Char(char);
    > ~Char();
    > void addPoint(int, int);
    > std::vector<Poi nt>& const getPixelPoints( );
    > char getChar() {return _ch;}
    > };
    >
    > Char::Char(char ch) :
    > _ch(ch)
    > { }
    >
    > Char::~Char() {
    > std::vector<Poi nt>::iterator iter;
    > for (iter=_pixelPoi nts.begin(); iter!=_pixelPoi nts.end();
    > iter++)
    > delete iter;
    > }[/color]

    Don't need the destructor, remove it.
    [color=blue]
    >
    > void Char::addPoint( int x, int y) {
    > _pixelPoints.pu sh_back(*( new Point(x,y) ));[/color]

    Don't need to use new

    _pixelPoints.pu sh_back(Point(x ,y));
    [color=blue]
    > }
    >
    > std::vector<Poi nt>& const Char::getPixelP oints() {
    > return _pixelPoints;
    > }
    >[/color]

    Should be fine now.

    john


    Comment

    • John Harrison

      #3
      Re: simple delete question


      "John Harrison" <john_andronicu s@hotmail.com> wrote in message
      news:2h3khnF8dv fsU1@uni-berlin.de...[color=blue]
      >
      > "cppaddict" <hello@hello.co m> wrote in message
      > news:n46pa05qdj 8psnifg0t79jbop 20jledl89@4ax.c om...[color=green]
      > > 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.[/color]
      >
      > 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.
      >[/color]

      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


      Comment

      • Dario (drinking coffee in the office…)

        #4
        Re: simple delete question

        cppaddict wrote:
        [color=blue]
        > 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.[/color]

        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.pu sh_back(*p));
        delete p;
        }

        and avoid to delete anything in ~Char().

        Just my 2 cents...

        - Dario

        Comment

        • John Harrison

          #5
          Re: simple delete question


          "Dario (drinking coï¬?ee in the oï¬fceâ?¦)" <dario@despamme d.com> wrote in
          message news:c8i6nt$ahr $1@grillo.cs.in terbusiness.it. ..[color=blue]
          > cppaddict wrote:
          >[color=green]
          > > 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.[/color]
          >
          > 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.pu sh_back(*p));
          > delete p;
          > }
          >[/color]

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

          _pixelPoints.pu sh_back(Point(x ,y));

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

          john


          Comment

          • Dario (drinking coffee in the office…)

            #6
            Re: simple delete question

            John Harrison wrote:
            [color=blue]
            > "Dario (drinking coï¬?ee in the oï¬fceâ?¦)" <dario@despamme d.com> wrote in
            > message news:c8i6nt$ahr $1@grillo.cs.in terbusiness.it. ..
            >[color=green]
            >> cppaddict wrote:
            >>[color=darkred]
            >>> 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.[/color]
            >>
            >> 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.pu sh_back(*p));
            >> delete p;
            >> }[/color]
            >
            > That works but why?? Needless use of new, inefficient and error prone.
            >
            > _pixelPoints.pu sh_back(Point(x ,y));[/color]

            Good point.
            [color=blue]
            > What's so hard about the above that newbies fail to use it?[/color]

            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

            Comment

            • John Harrison

              #7
              Re: simple delete question


              "Dario (drinking co?ee in the o?ce.)" <dario@despamme d.com> wrote in message
              news:c8i7td$ddi $1@balena.cs.in terbusiness.it. ..[color=blue]
              > John Harrison wrote:
              >[color=green]
              > > "Dario (drinking coï¬?ee in the oï¬fceâ?¦)" <dario@despamme d.com> wrote[/color][/color]
              in[color=blue][color=green]
              > > message news:c8i6nt$ahr $1@grillo.cs.in terbusiness.it. ..
              > >[color=darkred]
              > >> 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.pu sh_back(*p));
              > >> delete p;
              > >> }[/color]
              > >
              > > That works but why?? Needless use of new, inefficient and error prone.
              > >
              > > _pixelPoints.pu sh_back(Point(x ,y));[/color]
              >
              > Good point.[/color]

              Ho ho!
              [color=blue]
              >[color=green]
              > > What's so hard about the above that newbies fail to use it?[/color]
              >
              > 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)
              >[/color]

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

              john


              Comment

              • Jeff Flinn

                #8
                Re: simple delete question


                "John Harrison" <john_andronicu s@hotmail.com> wrote in message
                news:2h3mvtF8p0 14U1@uni-berlin.de...[color=blue]
                >
                > "Dario (drinking co?ee in the o?ce.)" <dario@despamme d.com> wrote in[/color]
                message[color=blue]
                > news:c8i7td$ddi $1@balena.cs.in terbusiness.it. ..[color=green]
                > > John Harrison wrote:
                > >[color=darkred]
                > > > "Dario (drinking coï¬?ee in the oï¬fceâ?¦)" <dario@despamme d.com>[/color][/color][/color]
                wrote
                [snip][color=blue][color=green]
                > >[color=darkred]
                > > > What's so hard about the above that newbies fail to use it?[/color]
                > >
                > > 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)
                > >[/color]
                >
                > Yes that explains some of it. But I definitely get the impression that
                > non-Java programmers also make similar mistakes.[/color]

                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


                Comment

                • Pete Becker

                  #9
                  Re: simple delete question

                  John Harrison wrote:[color=blue]
                  >
                  > 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.
                  >[/color]

                  Java pollution.

                  --

                  Pete Becker
                  Dinkumware, Ltd. (http://www.dinkumware.com)

                  Comment

                  • Stefan Pantos

                    #10
                    Re: simple delete question

                    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 <n46pa05qdj8psn ifg0t79jbop20jl edl89@4ax.com> cppaddict wrote:[color=blue]
                    > 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<Poi nt> _pixelPoints;
                    > public:
                    > Char(char);
                    > ~Char();
                    > void addPoint(int, int);
                    > std::vector<Poi nt>& const getPixelPoints( );
                    > char getChar() {return _ch;}
                    >};
                    >
                    > Char::Char(char ch) :
                    > _ch(ch)
                    > { }
                    >
                    > Char::~Char() {
                    > std::vector<Poi nt>::iterator iter;
                    > for (iter=_pixelPoi nts.begin(); iter!=_pixelPoi nts.end();
                    > iter++)
                    > delete iter;
                    >}
                    >
                    > void Char::addPoint( int x, int y) {
                    > _pixelPoints.pu sh_back(*( new Point(x,y) ));
                    >}
                    >
                    > std::vector<Poi nt>& const Char::getPixelP oints() {
                    > return _pixelPoints;
                    >}
                    >
                    >
                    >[/color]

                    Comment

                    • John Harrison

                      #11
                      Re: simple delete question


                      "Stefan Pantos" <stefan.pantos@ chem.ox.ac.uk> wrote in message
                      news:2004052014 5319531+0100@ne ws.ox.ac.uk...[color=blue]
                      > 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.
                      >[/color]

                      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


                      Comment

                      • cppaddict

                        #12
                        Re: simple delete question

                        >I'm curious as to what led you to use new at all. After all there are no[color=blue]
                        >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.[/color]

                        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==>dynam ic 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

                        Comment

                        • Howard

                          #13
                          Re: simple delete question

                          >[color=blue]
                          > That works but why?? Needless use of new, inefficient and error prone.
                          >
                          > _pixelPoints.pu sh_back(Point(x ,y));
                          >
                          > What's so hard about the above that newbies fail to use it?
                          >[/color]

                          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...rig ht?)

                          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








                          Comment

                          • John Harrison

                            #14
                            Re: simple delete question


                            "Howard" <alicebt@hotmai l.com> wrote in message
                            news:HQ5rc.8675 $fF3.211108@bgt nsc05-news.ops.worldn et.att.net...[color=blue][color=green]
                            > >
                            > > That works but why?? Needless use of new, inefficient and error prone.
                            > >
                            > > _pixelPoints.pu sh_back(Point(x ,y));
                            > >
                            > > What's so hard about the above that newbies fail to use it?
                            > >[/color]
                            >
                            > 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...rig ht?)
                            >[/color]

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

                            Point tmp(x, y);
                            _pixelPoints.pu sh_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


                            Comment

                            • John Harrison

                              #15
                              Re: simple delete question


                              "cppaddict" <hello@hello.co m> wrote in message
                              news:khnpa0pv4g kcm9jg8irqtgr1c i2evh1k64@4ax.c om...[color=blue][color=green]
                              > >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[/color][/color]
                              and[color=blue][color=green]
                              > >delete (and pointers generally) so attractive to newbies. Maybe you can
                              > >explain.[/color]
                              >
                              > 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==>dynam ic 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.[/color]

                              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(*(n ew int(5));

                              Or would you have just written

                              vector<int> x;
                              x.push_back(5);
                              [color=blue]
                              >
                              > 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.[/color]

                              No, definitely not, the cost of allocating large numbers of small objects is
                              going to greatly exceed the cost of copying those objects.
                              [color=blue]
                              >
                              > 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.[/color]

                              Right, so you were coding like this?

                              Char c;
                              c.addPoint(*(ne w 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(Poin t(x, y));

                              [color=blue]
                              >
                              > 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?
                              >[/color]

                              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


                              Comment

                              Working...