1
#include<iostream>
#include<conio.h>
using namespace std;
class A
{
public:
       int *p;      
A()
{
   p =new int;
}

~A()
{
delete p;   //Is this what i am doing is correct?
cout << "in A's destructor"<<endl;
}

};

int main()
{
A *obj=new A;
delete obj;    
getch();
}

This programs,i have executed in Dev c++ and compiles and executes fine. But i doubt this is not fine.Specially in the destructor where i say delete P

am i wrong?

Vijay
  • 65,327
  • 90
  • 227
  • 319
  • 1
    Your code is OK, your choice of IDE somewhat less so. DevC++ is a pile of junk - try Code::Blocks at http://www.codeblocks.org. –  Apr 26 '11 at 09:23
  • 3
    I would encourage you to expand on your doubts. Why do you feel the delete is not fine? – corsiKa Apr 26 '11 at 09:24
  • Just initialize `p` with `NULL` to avoid undefined behavior if `new p` throws, and let your `main()` function actually return an int value and you're good to go. – ognian Apr 26 '11 at 10:44
  • @ognian: there is no undefined behaviour here (if `new p` throws, then the destructor won't be invoked, and the failed object won't be accessible), and C++ allows you to omit the `return` statement from `main`. – Mike Seymour Apr 26 '11 at 12:33

4 Answers4

11

That code is logically fine (the part with new/delete you're asking about), but designed badly in other aspects.

First, if class A owns that heap-allocated int (int lives for as long as class A object lives) there's no point in creating it with new, it'd be much more efficient to make it a member variable of class A. Second, you shouldn't make it public - that breaks encapsulations and allows lots of abuse.

Finally, you still have copy constructor and assignment operator allowed and unimplemented, so any time a class object is copied you get shallow copy. You should either implement them appropriately or prohibit them.

The part where you delete a variable allocated with new is completely fine however.

Community
  • 1
  • 1
sharptooth
  • 167,383
  • 100
  • 513
  • 979
  • 1
    I would not say it's completely fine. Each `delete` is a memory leak or invalid memory use in waiting. Better to use smart pointers *every* time. – Matthieu M. Apr 26 '11 at 09:43
  • @Matthieu M.: A smart pointer is a class that does more or less the same as `class A` in questions, isn't it? – sharptooth Apr 26 '11 at 09:45
  • 1
    except that it is correctly implemented, code-reviewed, and proven to work. Also, you don't have to maintain it. – Matthieu M. Apr 26 '11 at 11:18
2

I see 4 issues with this code:

  • two uses of new
  • two uses of delete

It may be that your reduced code snipped is somewhat lacking wrt the original implementation, or that you come from a language where everything is new'ed, but the code as is is far from being idiomatic C++.

The idiomatic rewrite:

#include<iostream>

class A
{
public:
  int p;

  ~A() { std::cout << "~A" << std::endl; } // force a flush with endl
};

int main(int argc, char* argv[])
{
  {
    A obj;
  } // for destruction to occur before getch

  // use streams instead of getch
  char a;
  std::cin >> a;

  // the signature says it returns an int...
  return 0;
}
Matthieu M.
  • 287,565
  • 48
  • 449
  • 722
1

As sharptooth pointed out, using delete in the dtor is completely valid. delete null is defined in the std as noop.

however, for things like this, consider using a shared_ptr or something similar...

hth

Mario

Mario The Spoon
  • 4,799
  • 1
  • 24
  • 36
1

You also need to define your own copy constructor and assignment operator (or declare them as private to make the class as noncopyable) in this case; the default copy constructor and assignment operator do a shallow copy. That is, they copy the pointer, not the contents of the pointer. So if you do this:

A x;
A y = x;

Both x.p and y.p point to the same location, hence when they're destructed they try to free the same memory, which yields undefined behavior.

To fix this, define your own copy constructor and assignment operator that copies the int:

class A
{
public:
        A() : p(new int) {}
        A(const A& obj) : p(new int(*obj.p)) {}
        ~A() { delete p; }
        A& operator=(const A& obj) { *p = *obj.p; }
private:
        int *p;
};
reko_t
  • 55,302
  • 10
  • 87
  • 77