Skip to content
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

COMMON: Mempool checked build #2479

Closed
wants to merge 6 commits into from
Closed

Conversation

@henke37
Copy link
Contributor

@henke37 henke37 commented Sep 23, 2020

This PR adds validation to the memory pool system to force correct usage.

@henke37 henke37 force-pushed the henke37:mempoolchecker branch 2 times, most recently from 27a0a50 to ba65f2d Sep 23, 2020
@henke37 henke37 marked this pull request as ready for review Sep 25, 2020
@criezy
criezy approved these changes Sep 26, 2020
Copy link
Member

@criezy criezy left a comment

This looks good to me. I don't know how useful it will prove to be, but other than a slight performance decrease in debug builds it should no do any harm.

I would just wait on PR #2486 before merging this in case there was a good reason why we might not want to define NDEBUG in release builds.

@@ -26,6 +26,14 @@
#include "common/scummsys.h"
#include "common/array.h"

#ifndef NDEBUG

This comment has been minimized.

@criezy

criezy Sep 26, 2020
Member

We do not want to enable those checks in release builds, and currently we do not define NDEBUG in our release builds. I don't see any good reason for it, so I created PR #2486 to add it. But if it turns out there was a good reason not to define NDEBUG, this line will need to be changed to check RELEASE_BUILD instead.

This comment has been minimized.

@criezy

criezy Oct 4, 2020
Member

It looks like adding NDEBUG for release builds might not happen anytime soon. Can you please replace #ifndef NDEBUG with #ifndef RELEASE_BUILD and then I think we can merge this?

@henke37 henke37 force-pushed the henke37:mempoolchecker branch from ba65f2d to 1478af8 Oct 2, 2020
@sev-
Copy link
Member

@sev- sev- commented Jun 7, 2021

Closing this with no reaction. Moreover, error() on this kind of problem could lead to major regressions.

@sev- sev- closed this Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants