#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?
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.
delete is a memory leak or invalid memory use in waiting. Better to use smart pointers every time.
class A in questions, isn't it?I see 4 issues with this code:
newdeleteIt 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; } 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
delete null - if new fails an exception is thrown and the destructor is not even run.
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; }; 
obj is a reference, not a pointer. Neither (*obj).p or obj->p would even compile, you'd be trying to dereference a non-pointer.
pwithNULLto avoid undefined behavior ifnew pthrows, and let yourmain()function actually return an int value and you're good to go.new pthrows, then the destructor won't be invoked, and the failed object won't be accessible), and C++ allows you to omit thereturnstatement frommain.