0

I am currently learning / using c++, but I come from a Java background, so I apologize if this is a silly question. Below is some code representing the way I am handling errors generated by an external API. However, I am uncertain whether it would cause a memory leak when I assign a value to my error handling output parameter.

class ExceptionHandler {

private:
    std::string _msg;
    int _code;

public:
    ExceptionHandler(std::string msg = "", int code = 0) : 
         _msg(msg),
         _code(code)
    {
    }

    int code() {
        return _code;
    }
    std::string msg() {
        return _msg;
    }

}

 //This method returns true if it was executed with no errors
 //It returns false if an error occurred

    bool foo(ExceptionHandler * errHandler = NULL) {

        int sts; 

        //The API functions return 0 if completed successfully
        //else they returns some error code

        sts = some_api_func1();

        if(sts != 0) { //An error occurred!
            if(errHandler) {
                ExceptionHandler handler("Error from func1",sts);
                *errHandler = handler; //<--- Will this cause a memory leak since I would be losing errHandler's original value??
            }
            return false;
        }

        //My motivation for using exception handling this way is that I can 
        //set the handler's message based on what part it failed at and the 
        //code associated with it, like below:

        sts = some_api_func2();

        if(sts != 0) { //An error occurred!
            if(errHandler) {
                ExceptionHandler handler("Error from func2",sts); //<--- Different err message
                *errHandler = handler; //<--- But does this cause a memory leak?
            }
            return false;
        }

        return true;
    }


//Main method

int main() {
    ExceptionHandler handler;

    if(!foo(&handler)) {

        std::cout << "An exception occurred: (" << handler.code() << ") " << handler.msg() << std::endl;

    } else {

        std::cout << "Success!" << std::endl;

    }


}
  • Would the method 'foo()' cause a memory leak if an error occurred?

  • If so, how can I fix it? If not, how come it doesn't?

  • Is this a good way of handling errors?

Thank you in advance!

EDIT

I've learned that the above code would not generate a memory leak, but that the following code is a better way of handling errors (Thank you everyone!):

void foo() {

    int sts;

    sts = some_api_func1();


    if(sts != 0) 
        throw ExceptionHandler("Error at func1",sts);

    sts = some_api_func2();

    if(sts != 0)
        throw ExceptionHandler("Error at func2",sts);
}

int main() {

    try {
        foo();
        std::cout << "Success!";
    } catch(ExceptionHandler &e) { //<--- Catch by reference
        std::cout << "Exception: (" << e.code() << ") " << e.msg();
    }


}
souldzin
  • 1,428
  • 11
  • 23
  • 5
    Worse, handler is declared on the stack. As soon as your function returns, that memory is no longer valid and "bad" things will happen. Not a memory leak, but a segfault waiting to happen. – Michael Dorgan Feb 28 '13 at 22:14
  • 5
    You are aware that C++ has an exception mechanisms already, right? You can just throw an error. – andre Feb 28 '13 at 22:16
  • @Michael Dorgan I see, but my code still runs as expected even if 'foo' generates an error. Is there something that's happening in the background that I am not aware of? – souldzin Feb 28 '13 at 22:17
  • 1
    *"this way is that I can set the handler's message based on what part it failed at and the code associated with it"* - there are much more convenient ways of achieving the same. Simple `throw`ing of an exception would be sufficient here... there are ways how to even get a stack trace out of it so that you can see exactly the whole path that exception was "going" through... – LihO Feb 28 '13 at 22:18
  • 1
    I'm pretty sure you are just getting "lucky" or unlucky as the case may be. If nothing else happens to modify your stack where that memory exists, it will appear to work correctly. Using new to create a class on the heap and returning that will always work, but yes that will cause a leak if it is not later freed. BTW, smart pointers are the preferred way of handling that eventuality. – Michael Dorgan Feb 28 '13 at 22:20
  • 4
    The short answer to the question is you did not use the keyword `new` so you can't have a memory leak. – andre Feb 28 '13 at 22:20
  • @andre I read somewhere that when handling errors it is better to avoid using 'try-catch' and to handle it through output parameters. Maybe I misinterpreted it. Would you say that handling errors through 'try-catch-throw' would be more appropriate? – souldzin Feb 28 '13 at 22:21
  • 1
    Bah - my bad. You are passing in a class from the above function and "copying" the local structure into it. That will work - except for the whole copy constructor - shallow copy vs deep copy thing. Still get tripped up a bit with pointers after all these years... – Michael Dorgan Feb 28 '13 at 22:23
  • 1
    @SoulDZIN: *"I read somewhere that when handling errors it is better to avoid using 'try-catch' and to handle it through output parameters"* - What kind of errors?... The problem is that sometimes people abuse exceptions as an other way of passing some information through their application, which is really wrong... But this is not the case. Exceptions are perfectly reasonable here. – LihO Feb 28 '13 at 22:24
  • 2
    @SoulDZIN unless your working with tight constraints like an embedded system. Throwing an exception is the correct thing to do in this situation. – andre Feb 28 '13 at 22:24
  • @andre I see, I am planning on creating a .dll that I can use in Java. Would throwing exceptions cause anything that I should be aware of in this case? – souldzin Feb 28 '13 at 22:29
  • I would not know if that would cause an issue. – andre Feb 28 '13 at 22:33
  • K, I'll just have to find out. Thanks everyone! – souldzin Feb 28 '13 at 22:35
  • 1
    @SoulDZIN: Partially, C++ Exceptions cannot be allowed to "escape" into Java code. [See this question](http://stackoverflow.com/q/4138168/845092) – Mooing Duck Feb 28 '13 at 22:44
  • @MooingDuck Ok, thank you! That'll be good to know down the road. – souldzin Feb 28 '13 at 22:49
  • @SoulDZIN: Now when I see your edit, there's one more thing: ***throw by value, catch by reference***. I've edited my answer as well :) – LihO Feb 28 '13 at 23:09

1 Answers1

1
ExceptionHandler handler;
if (!foo(&handler)) {
    //...
}

defines an object with automatic storage duration, address of which is passed into the foo function. By the way, if you know you will always pass an argument to the function, then pass it by reference instead of pointer.

bool foo(ExceptionHandler * errHandler = NULL) {
    ExceptionHandler handler("Error in foo", 1);
    *errHandler = handler;
}

also creates another instance of ExceptionHandler with automatic storage duration, so this object is destroyed once the execution goes out of scope. But it's ok, since *errHandler = handler; uses a default assignment operator, that copies the values of handler's data members into the object that errHandler points to, so there's no memory leak in this case.

"Is this a good way of handling errors?"

No. This is not a proper way of handling errors. Use exceptions instead. Just make sure you don't end up abusing exceptions as an another way of passing the data through your program. So I recommend you to also have a look at: Why should exceptions be used conservatively?

Other relevant questions:
Is there a general consensus in the C++ community on when exceptions should be used?
"We do not use C++ exceptions" — What's the alternative? Let it crash?


Once you decide to use exceptions, make sure you throw by value and catch by reference:

if (...)
    throw MyException(...);

and somewhere:

try {
    ...
} catch (MyException& e) {
    ...
}
Community
  • 1
  • 1
LihO
  • 41,190
  • 11
  • 99
  • 167
  • Ok, is the reason for this so that the exception references the one thrown so that its not copied? – souldzin Mar 01 '13 at 15:39
  • @SoulDZIN: Yes, but the main reason behind is to avoid *slicing*. If you throw exception of type `MySpecialException1` but you would catch its base class by value: `catch (MyException e)`, then custom data members specific for `MySpecialException1` class will be lost. – LihO Mar 01 '13 at 15:45
  • Interesting, I will keep note of this and change my edit, thanks! – souldzin Mar 01 '13 at 15:48