The two remaining uses of
NDEBUG were in enabling/disabling the C macro
assert() and its old-LAL-style equivalent
ASSERT(). These macros are supposed to be used only for debugging code, i.e. code which is helpful for debugging, but is non-essential for the correct operation of the code, and can be removed without having any impact. Both macros are however commonly (mis)used for detecting error conditions (e.g. valid input arguments to functions, valid return values from functions), and even in test programs to check that test conditions are met. There is no reason such code should ever be disabled, as without such checks code could fail to detect errors, crash, or silently produce wrong results. This is particular true for production builds where
NDEBUG may be set.
ASSERT(), this macro is no longer enabled/disabled by
NDEBUG but by a new macro
LAL_ASSERT_MACRO_DISABLED. As documented in
LALStatusMacros.dox it is not recommended to compile LALSuite with this macro set, given that
ASSERT() is often used to detect critical errors (see #596 (closed) for an example of where disabling
ASSERT() leads to a change in behaviour). Nevertheless one can still disable
-DLAL_ASSERT_MACRO_DISABLED should anyone wish to do so.
assert(), this macro has been replaced with 2 new macros defined in
XLALError.h, which are never compiled away with
assertionis false. This macro has been used in library code, so that
lalAbortHook()can be replaced by e.g. the SWIG wrappings to prevent a call to
abort()(see !1945 (merged)). Ideally the
XLAL_CHECK...()macros which raise XLAL errors would be used here, but would that require me checking that
xlalErrnois being inspected for failures. Given that much of the use of
assert()is in LALSimulation and LALInference, where I'm not familiar with the code, I can't guarantee that
xlalErrnois being respected, so it seems safer to call
lalAbortHook()which preserves the same behaviour as
assert()(expect that the check is never compiled away).
assertionis false. This is fine to use in test programs where one generally wants to exit immediately if a test condition fails. It's more friendly for a test to fail with
abort()(again see !1945 (merged)).
To prevent usage of
assert() reappearing in LALSuite, I've added a
--enable-strict-defs, which when enabled uses
#pragma GCC poison to prevent any usage of
assert(). This is consistent with the LAL Specification and Style Guide which does not allow
assert() in LAL code.
--enable-strict-defs is disabled by default, so
assert() can be still used for local debugging, but is enabled in CI to prevent
assert() being (mis)used in production code.
To check that LALSuite now compiles with
NDEBUG I've removed
-UNDEBUG from the Conda builds (which unfortunately will probably conflict with !1975 (closed)). Given that LALSuite should now not depend on whether
NDEBUG is set, a separate CI job (!1971 (closed)) shouldn't be needed.
Fixes #596 (closed)
API Changes and Justification
Backwards Compatible Changes
This change does not modify any class/function/struct/type definitions in a public C header file or any Python class/function definitions
This change adds new classes/functions/structs/types to a public C header file or Python module
Backwards Incompatible Changes
This change modifies an existing class/function/struct/type definition in a public C header file or Python module
This change removes an existing class/function/struct/type from a public C header file or Python module