Remove last usages of NDEBUG and assert()
Description
This MR continues from !1930 (merged) and !1938 (merged) in removing the last usages of NDEBUG
from LALSuite.
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.
For 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 ASSERT()
with -DLAL_ASSERT_MACRO_DISABLED
should anyone wish to do so.
For assert()
, this macro has been replaced with 2 new macros defined in XLALError.h
, which are never compiled away with NDEBUG
:
-
XLAL_CHECK_ABORT(assertion)
callslalAbortHook()
ifassertion
is false. This macro has been used in library code, so thatlalAbortHook()
can be replaced by e.g. the SWIG wrappings to prevent a call toabort()
(see !1945 (merged)). Ideally theXLAL_CHECK...()
macros which raise XLAL errors would be used here, but would that require me checking thatxlalErrno
is being inspected for failures. Given that much of the use ofassert()
is in LALSimulation and LALInference, where I'm not familiar with the code, I can't guarantee thatxlalErrno
is being respected, so it seems safer to calllalAbortHook()
which preserves the same behaviour asassert()
(expect that the check is never compiled away). -
XLAL_CHECK_EXIT(assertion)
callsexit(1)
ifassertion
is 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 withexit()
thanabort()
(again see !1945 (merged)).
To prevent usage of assert()
reappearing in LALSuite, I've added a ./configure
option --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