New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Sanity Check for out of memory #894

Merged
merged 1 commit into from Nov 17, 2014

Conversation

Projects
None yet
6 participants
@Winterleaf
Contributor

Winterleaf commented Nov 4, 2014

Just a quick check for out of memory

@crabmusket crabmusket added the Defect label Nov 4, 2014

@crabmusket crabmusket added this to the 3.6.3 milestone Nov 4, 2014

@Areloch

This comment has been minimized.

Show comment
Hide comment
@Areloch

Areloch Nov 5, 2014

Contributor

Would it be prudent to have a error to the console so the end user is aware there is an out of memory issue, rather than failing silently?

Or is it such a stupidly unlikely case it's not worth the effort?

Contributor

Areloch commented Nov 5, 2014

Would it be prudent to have a error to the console so the end user is aware there is an out of memory issue, rather than failing silently?

Or is it such a stupidly unlikely case it's not worth the effort?

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Nov 5, 2014

Contributor

A warning sounds like a good idea. @Winterleaf is this intended to catch problems reading huge files? Not just, you know, your everyday out of memory issues, which are going to manifest catastrophically with or without this little guard.

Contributor

crabmusket commented Nov 5, 2014

A warning sounds like a good idea. @Winterleaf is this intended to catch problems reading huge files? Not just, you know, your everyday out of memory issues, which are going to manifest catastrophically with or without this little guard.

@LuisAntonRebollo

This comment has been minimized.

Show comment
Hide comment
@LuisAntonRebollo

LuisAntonRebollo Nov 5, 2014

Contributor

c++ new don't return nullptr on bad allocation, throw a std::bad_alloc exception.

For return nullptr we need to use new (std::nothrow) TYPE

See http://www.cplusplus.com/reference/new/operator%20new/

I prefer see Out of Memory as a fatal error, send a msg and close app. If not we are going to have same problem on each posterior allocation.

Contributor

LuisAntonRebollo commented Nov 5, 2014

c++ new don't return nullptr on bad allocation, throw a std::bad_alloc exception.

For return nullptr we need to use new (std::nothrow) TYPE

See http://www.cplusplus.com/reference/new/operator%20new/

I prefer see Out of Memory as a fatal error, send a msg and close app. If not we are going to have same problem on each posterior allocation.

@dottools

This comment has been minimized.

Show comment
Hide comment
@dottools

dottools Nov 5, 2014

@LuisAntonRebollo I don't believe it is reasonable to just abort on failure to allocate heap that's used to load an entire file to memory. It should just continue fail gracefully like it's doing now and let whoever called ReadFile() deal with it like it should be.

@eightyeight Are we allowing C++11 only code into T3D now?

dottools commented Nov 5, 2014

@LuisAntonRebollo I don't believe it is reasonable to just abort on failure to allocate heap that's used to load an entire file to memory. It should just continue fail gracefully like it's doing now and let whoever called ReadFile() deal with it like it should be.

@eightyeight Are we allowing C++11 only code into T3D now?

@Winterleaf

This comment has been minimized.

Show comment
Hide comment
@Winterleaf

Winterleaf Nov 5, 2014

Contributor

It is #defined out, so it shouldn't affect compiling on linux.

Contributor

Winterleaf commented Nov 5, 2014

It is #defined out, so it shouldn't affect compiling on linux.

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Nov 5, 2014

Contributor

@dottools I think the graceful failure is good in this case. As for C++11 code, are you referring to the SimDictionary changes? I figured that as it's an optional feature and disabled by default, yes, it was okay. @Winterleaf it's also an issue for earlier VS compilers.

Contributor

crabmusket commented Nov 5, 2014

@dottools I think the graceful failure is good in this case. As for C++11 code, are you referring to the SimDictionary changes? I figured that as it's an optional feature and disabled by default, yes, it was okay. @Winterleaf it's also an issue for earlier VS compilers.

tdev pushed a commit that referenced this pull request Nov 17, 2014

Thomas Fischer
Merge pull request #894 from Winterleaf/Dev---Volume.cpp
Added Sanity Check for out of memory

@tdev tdev merged commit e7a4d61 into GarageGames:development Nov 17, 2014

1 check passed

default Merged build finished.
Details
@tdev

This comment has been minimized.

Show comment
Hide comment
@tdev

tdev Nov 17, 2014

yep, agreed, better fail graceful in here.

tdev commented Nov 17, 2014

yep, agreed, better fail graceful in here.

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Nov 17, 2014

Contributor

Would have been nice to have it in 3.6.3, but oh well.

Contributor

crabmusket commented Nov 17, 2014

Would have been nice to have it in 3.6.3, but oh well.

@crabmusket crabmusket modified the milestones: 3.7, 3.6.3 Nov 17, 2014

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Nov 17, 2014

Contributor

Oh, and @tdev Luis was right, we need to specify nothrow in order to ever actually get a null pointer. I shouldn't have added it to final review, that was a mistake.

Contributor

crabmusket commented Nov 17, 2014

Oh, and @tdev Luis was right, we need to specify nothrow in order to ever actually get a null pointer. I shouldn't have added it to final review, that was a mistake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment