Skip to content
This repository has been archived by the owner on Mar 8, 2023. It is now read-only.

capture bad_alloc if it ever happens #46

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adrpar
Copy link
Contributor

@adrpar adrpar commented Dec 12, 2018

however usually the OOM killer steps in before that...

@adrpar adrpar requested a review from fester December 12, 2018 09:25
Copy link
Contributor

@fester fester left a 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();
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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];
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

@adrpar
Copy link
Contributor Author

adrpar commented Dec 12, 2018

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.

@adrpar adrpar force-pushed the nice_error_on_oom branch 2 times, most recently from c2d3e8d to e46dc57 Compare December 13, 2018 14:45
if("zemlya" == meshing_method || "terra" == meshing_method)
std::unique_ptr<char[]> emergency_memory(new char[16384]);

try
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants