Skip to content

Remove last usages of NDEBUG and assert()

Karl Wette requested to merge ANU-CGA/lalsuite:remove-ndebug into master

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) calls lalAbortHook() if assertion is 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 xlalErrno is 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 xlalErrno is being respected, so it seems safer to call lalAbortHook() which preserves the same behaviour as assert() (expect that the check is never compiled away).
  • XLAL_CHECK_EXIT(assertion) calls exit(1) if assertion 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 with exit() than abort() (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

Review Status

cc @duncanmmacleod @jolien-creighton @adam-mercer

Edited by Karl Wette

Merge request reports