-
Notifications
You must be signed in to change notification settings - Fork 127
capture bad_alloc if it ever happens #46
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please elaborate why did you added the two main_loop_* functions? Wasn't it enough just to catch a bad_alloc somewhere in the main()?
@@ -150,30 +201,6 @@ static int subcommand_dem2tintiles(bool need_help, | |||
if("zemlya" == meshing_method || "terra" == meshing_method) | |||
{ | |||
max_error = input_raster->get_cell_size(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this line will always erase a max_error value passed throught CLI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that was the case in the original code. So I guess that was on purpose? I cannot tell if that was intentional or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, completely messed this up. fixed.
src/cmd.cpp
Outdated
throw po::error(std::string("unknown method ") + meshing_method); | ||
} | ||
|
||
char* emergency_memory = new char[16384]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please clarify this buffer allocation? Also you don't free it when no bad_alloc happens. In order to be consistent with the rest of codebase, please use unique_ptr instead of a raw pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for this is, that if an OOM happens, the system might not have enough memory to actually handle and print the error message. Therefore 16k are allocated here, to be freed later (will add the free at the end of the code) to have enough memory to print the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved this to unique_ptr and added the same logic further below where I forgot to add it
I added the two main loop functions to reduce the width of how open the try catch will be. And to separate all the stuff needed to actually run the code from input argument parsing and validation (which could be even more separated IMO). Will address the comments tomorrow. |
c2d3e8d
to
e46dc57
Compare
…steps in before that... Signed-off-by: Adrian Partl <[email protected]>
e46dc57
to
d567a88
Compare
if("zemlya" == meshing_method || "terra" == meshing_method) | ||
std::unique_ptr<char[]> emergency_memory(new char[16384]); | ||
|
||
try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wonder if this has performance cost in our case https://stackoverflow.com/questions/1897940/in-what-ways-do-c-exceptions-slow-down-code-when-there-are-no-exceptions-thown
however usually the OOM killer steps in before that...