CesiumGS / cesium-native

Apache License 2.0
421 stars 212 forks source link

Add ability to force assertions #874

Closed csciguy8 closed 3 months ago

csciguy8 commented 5 months ago

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.

kring commented 4 months ago

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.

csciguy8 commented 3 months ago

So even though it's more verbose, we should should probably namespace the name itself a bit. CESIUM_ASSERT, perhaps?

Sounds reasonable. Namespace conflicts looks to be something that our external deps also consider...

#    define FMT_ASSERT(condition, message)              // spdlog
#  define Assert(cond,msg) {if(!(cond)) z_error(msg);} // zlib
#  define LIBASYNC_ASSERT(pred, except, message)  //asyncplusplus
#define DRACO_DCHECK(x) (assert(x));                       // draco

We could do something more sophisticated, like make a new assert-like function in the Cesium namespace, but I'm leaning toward a renaming. It's simple and fits the previous patterns too.

So what to rename it too? CESIUM_ASSERT looks fine, but a little long, like you said.

CASSERT? That looks too much like we're wrapping a compile time assert CN_ASSERT? As in cesium-native assert, hmm, we don't really have this convention anywhere CESIUM_CHECK? Saves just one character, nah

CESIUM_ASSERT it is!

csciguy8 commented 3 months ago

@kring , ready for another look