C++ exception handling

Question

I'm getting throw exception and need your review

The main function bellow trying to allocate memory several times and then throw the exception on the upper level.

#include <iostream> #include <memory> struct MiserlinessClass{ char * pMemory; int memory_len; const int max_size = 10; MiserlinessClass(int len){ if (len>max_size){ std::cout<<"What a lavish lifestyle! Get out of my face! \n"; std::bad_alloc exception; throw exception; } pMemory = (char *)malloc(len*sizeof(char)); memory_len = len; } }; int main(int argc, char** argv){ int len = (argc==2)? strtol(argv[1],NULL,10): 5; std::unique_ptr<MiserlinessClass> objPtr; bool allocated = false; const int max_cnt = 5; int cnt = 0; while (!allocated){ try{ std::cout<<"Trying to allocate "<<len<<" chars...\n"; objPtr.reset(new MiserlinessClass(len)); allocated = true; } catch (std::bad_alloc &e){ len = len >> 1; cnt++; if (cnt==max_cnt){ std::cout<<"I give up \n"; throw e; } } } std::cout<< "Allocated " << objPtr->memory_len << " chars \n"; return 0; } 

And here are the results of 3 different runs

1 - allocates memory from the very first attempt,

2 - tries few times and allocates available and T

3 - failed after N attempts and throw an exception to the upper level

--------------------------------------- $ make; ./01_exception_pointer 3 make: Nothing to be done for 'all'. Trying to allocate 3 chars... Allocated 3 chars --------------------------------------- $ make; ./01_exception_pointer 64 make: Nothing to be done for 'all'. Trying to allocate 64 chars... What a lavish lifestyle! Get out of my face! Trying to allocate 32 chars... What a lavish lifestyle! Get out of my face! Trying to allocate 16 chars... What a lavish lifestyle! Get out of my face! Trying to allocate 8 chars... Allocated 8 chars --------------------------------------- $ make; ./01_exception_pointer 1023 make: Nothing to be done for 'all'. Trying to allocate 1023 chars... What a lavish lifestyle! Get out of my face! Trying to allocate 511 chars... What a lavish lifestyle! Get out of my face! Trying to allocate 255 chars... What a lavish lifestyle! Get out of my face! Trying to allocate 127 chars... What a lavish lifestyle! Get out of my face! Trying to allocate 63 chars... What a lavish lifestyle! Get out of my face! I give up terminate called after throwing an instance of 'std::bad_alloc' what(): std::bad_alloc Aborted (core dumped) 

My questions are

1. I know the formal difference between returning value and throwing the exception, but still can not feel whether it's time to panic and throw an exception or the program should keep calm and just return the error code.

2. Is is a proper way to handle bad_alloc exception?

3. Any extra comments and suggestions :)

Answer 1

I know the formal difference between returning value and throwing the exception, but still can not feel whether it's time to panic and throw an exception or the program should keep calm and just return the error code.

Simple Rules of thumb.

• If you can not fix the issue locally then throw an exception.
  You can not solve the problem of not having enough memory locally so throw an exception. This will allow the stack to unwind memory be released and you "May" get to a point where this can be solved (or if not let the application exit). An example of where it can be stopped is when you are creating independent task and one of these tasks fails. It does not mean that all tasks will fail. Log the fact that this task has failed allow the exception to release all the memory it used and then try the next task.

• Error code should not cross interface boundaries.
  Error codes are great if you check them. It allows a simple mechanism to pass information back one or two levels without complicating the code. So if you are writing a library and internally you use error codes that is fine (because you will be good and check all error codes). But you can not trust users of your library so if an error is going to propagate outside your library use an exception to force the user to explicitly get it.

• User Input and streams. Don't use exceptions. User input is always going to be error prone and the code that handles user input is going to need to have lots of validation checks (if done correctly). They know this. So on stream operations simply set the stream to bad.

Is a proper way to handle bad_alloc exception?

Let it propagate to the top of your app. Log something to let the user know it happened. If this is an independent task drop the task and start the next one. If this is just a normal part of executing then let the application exit.

Any extra comments and suggestions :)

• Don't use malloc()/free() in C++ code.
• For every new there should be a delete.
• Prefer to use make_unique() rather than new (to help with new/delete matching).

Code Review:

// Owned pointers are a bad idea. struct MiserlinessClass{ char * pMemory; // This is an owned pointer. // At lot of extra work needs to be done here // You need to look up the rule of three/five 

---

 std::bad_alloc exception; throw exception; 

Easier to simply write:

 throw std::bad_alloc; 

---

This is not a bad_alloc situation.

 if (len>max_size){ std::cout<<"What a lavish lifestyle! Get out of my face! \n"; std::bad_alloc exception; throw exception; } 

bad_alloc means that the system failed to allocate memory because of memory pressure.

You should use: std::range_error the input parameter was out of range.

---

Don't use malloc use new here.

 pMemory = (char *)malloc(len*sizeof(char)); // Better: pMemory = new char[len]; 

Note: You still need to implement the rule of three here.

---

Note: Yes you should be throwing an exception above. You do not want to allow the user to create invalid objects. This needs to be fixed before the application is allowed into production so forcing the unit test to fail with an exception is the correct solution.

---

There is no destructor to the class MiserlinessClass. So the memory allocated with malloc() is going to be leaked. Forcing you to run out of memory quicker.

See Rule of three.

---

Note: This will release the previous object. But only if the new object is successfully created. So you have a bunch of memory allocated. Then you try and allocate twice as much. If that allocation works then you release the old memory.

 objPtr.reset(new MiserlinessClass(len)); 

I would move the declaration:

 std::unique_ptr<MiserlinessClass> objPtr; 

into the try block do the allocation there.

 try{ std::cout<<"Trying to allocate "<<len<<" chars...\n"; std::unique_ptr<MiserlinessClass> objPtr = std::make_unique<MiserlinessClass>(len); allocated = true; } catch (std::bad_alloc &e){ ... 

Now. The memory is allocated and then released at the end of the try. Next time around the loop you know that you have clean store to allocate from as everything was cleaned up from your last attempt.

You could go a step further and simply remove the unique_ptr.

 try{ std::cout<<"Trying to allocate "<<len<<" chars...\n"; MiserlinessClass objPtr(len); allocated = true; } catch (std::bad_alloc &e){ ... 

---

Why not just multiply by 2.

 len = len >> 1; 

Or is that divide by 2. Either way the intent is not clear. Use code that expresses your intent clearly.

---

Only prints if there is no exception.

 std::cout<< "Allocated " << objPtr->memory_len << " chars \n";