0

According to the following article: Why isn’t the destructor called at the end of scope?

Code that creates an object using new and then deletes it at the end of the same scope is ugly, error-prone, inefficient, and usually not exception-safe. For example:

 void very_bad_func()    // ugly, error-prone, and inefficient
    {
        X* p = new X;
        // use p
        delete p;  // not exception-safe
    }

My Code which I hope is not ugly:

I am creating an object of type TiXmlDocumentand the delete it by the end of the function.

void DataLoader::readXmlFile(const string & file)
{
    TiXmlDocument *doc = new TiXmlDocument(file.c_str());
    bool loadOkay = doc->LoadFile();
    TiXmlHandle hdl(doc);

    //work is done in here
    if(loadOkay)
    {
        TiXmlElement * pRoot = hdl.FirstChildElement().Element();//.FirstChildElement().Element();
        parseFile(pRoot);
    }
    else
    {
        cout <<"Error: "<< doc->ErrorDesc()<<endl;
    }
    //deallocate doc
    delete doc;
}

Question(s):

  • Should I use DataLoader::~DataLoader() {} destructor in order to insure that the object is deleted after I leave the scope of the function? without the need of explicitly deleting it delete doc.

As suggested I did the following:

TiXmlDocument doc(xmlFile.c_str());
bool loadOkay = doc.LoadFile();
TiXmlHandle hdl(&doc);

I am starting to think that using dynamic memory just like java and c# is not a good practice (should be used in a responsible way) in c++. If there is no real cause to use it then don't. If not handled correctly it will cause a memory leak which is hard to trace.

Community
  • 1
  • 1
Hani Goc
  • 2,371
  • 5
  • 45
  • 89
  • Do you mean something like [smart pointers](http://msdn.microsoft.com/en-us/library/hh279674.aspx)? `std::unique_ptr doc(new TiXmlDocument(file.c_str()));` – Andreas Vennström Mar 23 '15 at 13:02

2 Answers2

7

Either create your object without new:

void func()
{
    X p;
    // use p
}

Or if you must use new (for example, it's a big object) then use a smart pointer:

void func()
{
    std::unique_ptr<X> p(new X);
    // use p
}

Problem solved!

Neil Kirk
  • 21,327
  • 9
  • 53
  • 91
3

That's still ugly (in my view), and certainly error-prone, inefficient, and not exception-safe. If an exception is thrown during this function, the delete won't happen, and you'll leak memory.

In this case, there's no need for new at all; use an automatic variable, which will be destroyed automatically when the function exits, whether by reaching the end, returning, or throwing.

TiXmlDocument doc(file.c_str());

If you did need dynamic allocation for some reason, then use a smart pointer or other RAII type to get the same automatic behaviour:

auto doc = std::make_unique<TiXmlDocument>(file.c_str());            // C++14 or later
std::unique_ptr<TiXmlDocument> doc(new TiXmlDocument(file.c_str())); // C++11

To answer the last question: use the destructor to deallocate resources managed by the class instance, not transient resources used within a function call. Again, you should usually use RAII types as member variables to manage these resources automatically, so that you don't need to write a destructor yourself.

Community
  • 1
  • 1
Mike Seymour
  • 249,747
  • 28
  • 448
  • 644
  • @HaniGoc: Yes, if you still want to use `new` rather than an automatic variable for some reason, that looks right, assuming `TiXmlHandle` doesn't do something silly like delete the pointer that it's given. – Mike Seymour Mar 23 '15 at 13:13
  • I don't know what `TiXmlHandle`will do that's the problem. So I think it's better as you said to use smart pointers right? – Hani Goc Mar 23 '15 at 13:15
  • 1
    @HaniGoc: If you don't know whether `TiXmlHandle` might try to delete it, then there's nothing you can safely do. You'll need to find out why it takes a pointer, and whether or not it expects to take ownership. If it does expect to delete it, then you'll need `new` with no `delete` (and no smart pointer). If it doesn't, then there's no good reason to use `new` at all - make it a local variable. – Mike Seymour Mar 23 '15 at 13:17
  • 2
    @HaniGoc If you don't know what a function will do with a pointer you pass it, then you shouldn't call the function.. – Neil Kirk Mar 23 '15 at 13:59