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

Add ability to force assertions #874

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add ability to force assertions #874

wants to merge 5 commits into from

Conversation

csciguy8
Copy link
Contributor

@csciguy8 csciguy8 commented May 3, 2024

This adds the ability for cesium-native asserts to execute in configurations where they are usually turned off.

A good example is with cesium-unreal. Unreal Engine lets their plugins run in Debug or Release, but the engine is built in Release (with NDEBUG present). Assertions get compiled out either way. In this situation, we can use the new CESIUM_FORCE_ASSERTIONS define with our debug build, to see debug assertions like we expect. This addresses cesium-unreal #1366.

To use, the native app needs to build cesium-native with the CESIUM_FORCE_ASSERTIONS preprocessor present. If defining this for a configuration that would normally have assertions anyway, it is ignored.

A side benefit is that we can now force assertions in Release builds. This could be useful in some situations, like when trying to diagnose bugs that happen in Release only.

#pragma once

//
// Define our own assertion, so that users can forcebly turn them on if needed
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good place to start a review

@kring
Copy link
Member

kring commented Jun 4, 2024

This looks good to me @csciguy8. The only small reservation I have is the name ASSERT. Macros can't be namespaced, so we're basically claiming this name globally. There's significant risk of a conflict. So even though it's more verbose, we should should probably namespace the name itself a bit. CESIUM_ASSERT, perhaps?

There's also a slightly different approach used by another project internal to Cesium that might be worth considering. I'll send you a link to that separately.

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

Successfully merging this pull request may close these issues.

None yet

2 participants