Thread: Deleting an object if the constructor failed.

Hi guys, I want to ask you about how to delete an object if its constructor failed to initialize one of its members. I tried it like below but failed:

Code:

B* A:CreateB(std::string paramStr)
{
    B* pB= NULL;
    try
    {
         pB = new pB (paramStr);
    }
    catch (int result)
    {
         if (result == OK) // OK == 1
         {
                m_VectorB.push_back(pB);
         }
          else if (result == FAILED)
         {
                delete pB;
                pB = NULL;
         }
    }
    return pB;
}

B::B(std::string paramStr)
{
    int result = initMembers (paramStr); //returns OK / 1 if successful or FAILED / 0 if failed
    throw result ;
}

Now, FYI, I have never ever ever used exception in my entire life as a programmer. All my error handlings are either by return value or assertion. I know that it is not a good habit but I still don't have a good understanding about it. So, how can I solve this problem? Thanks in advance.

new does two things: a) allocate memory, b) call constructor. delete does the opposite: a) call destructor, b) deallocate memory.

Now if the constructor throws an error, new deallocates the memory and rethrows the exception. Also, when the constructor throws, the object never comes to existence (members initialized so far are destroyed) and therefore it would be an error to invoke the destructor on the zombie.

So your attempt to signal success by throwing an exception from a constructor couldn't possibly work.

Correct way:

Code:

B* A::CreateB(std::string paramStr)
{
   //no special error handling needed here
   //some caller handles allocation/construction failures
    B* pB = new pB (paramStr);

    //if the previous line didn't throw (= if we get this far in the first place), everything's OK
    m_VectorB.push_back(pB);
         
    return pB;
}

B::B(std::string paramStr)
{
    int result = initMembers (paramStr); //returns OK / 1 if successful or FAILED / 0 if failed
    if (!result) throw some_exception("something is wrong"); //only throw in case of failure
}

Of course, even better would be to have each member initializer in turn use exceptions to signal errors, because the entire point of exceptions is that they propagate upwards automatically.

I suggest that you read GotW #66: Constructor Failures.

Thanks for the reply guys

@Sebastiani: Well, I know that the 'new' method mallocs and then calls the constructor and 'delete' calls the destructor and then free the memory. What I don't know is how exception works in general. And I thought that the object will be fully created even if I throw an exception in the constructor. I guess I'm wrong.

About passing int as an exception, yeah, I know that it's not a good idea. But it is sufficient as of now.

BTW, as for assigning 'new' to a raw pointer, I've seen lots of open source libraries do that also. So i thought, why can't I?

@anon: OIC, I thought we can throw anything at anytime. My mistake then. BTW, your code didn;t work. I tried this one though and it worked:

Code:

B* A:CreateB(std::string paramStr)
{
    B* pB= NULL;
    try
    {
         pB = new pB (paramStr);
    }
    catch (int result)
    {
          pB = NULL;
    }

    m_VectorB.push_back(pB);
    return pB;
}

B::B(std::string paramStr)
{
    int result = initMembers (paramStr); //returns OK / 1 if successful or FAILED / 0 if failed
    if (result == FAILED)
                   throw result ;
}

@CornedBee: Well I would if there's no time constraint. Heck, I will rewrite the entire code with exceptions if there's time. But for now, it's okay to do this quick hack. :P

@laserlight: Thanks. Another source of information is always good.

Doing exceptions without strictly following RAII principles is just setting yourself up for lots and lots of headaches, and bugs in code paths that are very rarely tested.

Originally Posted by g4j31a5

BTW, as for assigning 'new' to a raw pointer, I've seen lots of open source libraries do that also. So i thought, why can't I?

I commonly assign new to LPVOID when I am abstracting a class to hide internal details when distributing a closed source middelware library. This way I can have a header with only the public methods and data showing, enforcing privatization and at the same time protecting trade secrets.

e.g.

Code:

class Public_Class {
   LPVOID pObject;

   // public functions and data go here

then in the actual constructor

Code:

Public_Class::Public_Class(){

   this->pObject = (LPVOID) new ActualClass();

   return;
   }

Since my public class functions will have access tot he ActualClass header they can manipulate the object by recasting pObject to (ActualClass*), but the end user only gets the compiled library and the Public_Class header, so they do not have access to the ActualClass member functions or variables and can only access it through the methods I allow.

But why not do this?

Code:

class Public_Class {
  class Impl;
  boost::scoped_ptr<Impl> m_impl; // or equivalent to scoped_ptr

public:
  Public_Class();
  ~Public_Class();
};

// implementation:
class Public_Class::Impl
{
  // ...
};

Public_Class::Public_Class()
  : m_impl(new Impl)
{}
Public_Class::~Public_Class()
{} // Empty. It's only out-of-line to force an instantiation location for scoped_ptr's destructor.

No casting, no chance of exception unsafety. You have to implement copying if you want it, of course, but you have to do that anyway.

Not my intention to hijack the topic - but the reason you shouldn't be using new directly with raw pointers with custom objects is more or less because it never goes out of scope, due to the simple fact that it's not allocated on the stack? And using std::auto_ptr solves this for us because when it goes out of scope, it destroys the objects it is encapsulating?

Very informative post - still learning how to write proper code in C++.

but the reason you shouldn't be using new directly with raw pointers with custom objects is more or less because it never goes out of scope, due to the simple fact that it's not allocated on the stack?

More generally, you shouldn't do it because you have to explicitly control (and document) the lifetime and ownership of the object, whereas smart pointers automate, enforce and document a specific ownership and lifetime model. For example, scoped_ptr enforces single, local ownership with scoped lifetime. If you see a scoped_ptr, you know that this is the case. If you see a raw pointer, you always have to wonder.

Originally Posted by CornedBee

More generally, you shouldn't do it because you have to explicitly control (and document) the lifetime and ownership of the object, whereas smart pointers automate, enforce and document a specific ownership and lifetime model. For example, scoped_ptr enforces single, local ownership with scoped lifetime. If you see a scoped_ptr, you know that this is the case. If you see a raw pointer, you always have to wonder.

I see. Thanks =)

I was actually wondering how I should go about solving a problem, and by surfing this forum, not looking for the solution at all, I stumbled upon it, heh.

@Sebastiani & CornedBee: RAII? what's that? sorry that's still new to my ear. I swear, the profs have never mentioned that word when I was in college. -_-a

@abachler: LPVOID is the same as void*, right? Although it's doable, it's too much of a hassle IMHO. I rarely use void* so that the code can be read better. :P

You know, I've also never used auto_ptr. It's not portable IIRC. Maybe I'll try it in the future. As for passing pointers out of scope, I usually does that and control them strictly by myself. So I have to make sure if the object is still alive or not before doing anything to it on different objects. I usually code what came up first in my head. I confess, I suck at designing my codes. :P

Originally Posted by g4j31a5

You know, I've also never used auto_ptr. It's not portable IIRC.

It is part of the C++ standard library.