BTAS_ASSERT_POLICY: real THROW / ABORT / IGNORE modes

[evaleev]

Motivation

BTAS_ASSERT has had 2 modes, selected by the boolean BTAS_ASSERT_THROWS: throw btas::exception (ON), or a plain assert() (OFF). The OFF mode conflates two distinct behaviors — abort and do-nothing — and lets NDEBUG pick between them, so there is no way to ask BTAS for "assertions are no-ops" or for "assertions abort" independently of the build type.

This blocks assert-policy forwarding in the dependents:

• TiledArray cmake: forward TA_ASSERT_POLICY to BTAS built from source tiledarray#587 forwards TA_ASSERT_POLICY (TA_ASSERT_{THROW,ABORT,IGNORE}) to BTAS
• SeQuant cmake: forward SEQUANT_ASSERT_BEHAVIOR to TiledArray and BTAS built from source SeQuant#616 forwards SEQUANT_ASSERT_BEHAVIOR (THROW/ABORT/IGNORE) to TiledArray and BTAS

Both could only map IGNORE and ABORT onto BTAS_ASSERT_THROWS=OFF, i.e. onto assert() — which, in a Debug build, still aborts when the user asked for IGNORE (this is exactly what Copilot flagged on both PRs). A real 3-mode policy in BTAS is the proper fix.

What this does

Adds the BTAS_ASSERT_POLICY cache variable, named and valued after TiledArray's TA_ASSERT_POLICY:

BTAS_ASSERT_POLICY	BTAS_ASSERT(x) if x is false
BTAS_ASSERT_THROW	throws btas::exception (what BTAS_ASSERT_THROWS=ON did)
BTAS_ASSERT_ABORT	reports the message to stderr and calls std::abort()
BTAS_ASSERT_IGNORE	nothing; expands to do {} while(0), so x is not evaluated

• None of the 3 depends on NDEBUG — in particular ABORT aborts in Release too, and IGNORE is a no-op in Debug.
• The message format of BTAS_EXCEPTION_MESSAGE is unchanged, except that the stringized assertion expression is now included.
• The choice is exported by the BTAS INTERFACE target as -DBTAS_ASSERT_POLICY=..., so consumers compile against the same policy. btas/error.h defaults to BTAS_ASSERT_ABORT if the macro is absent.
• Default: BTAS_ASSERT_THROW if BUILD_TESTING=ON (the unit tests check BTAS_ASSERT failures, hence must be able to catch them), else BTAS_ASSERT_IGNORE for Release/MinSizeRel and BTAS_ASSERT_ABORT otherwise. That preserves today's effective behavior in every configuration (assert() used to be elided under NDEBUG and to abort otherwise).

Compatibility

BTAS_ASSERT_THROWS is kept as a deprecated alias: if it is given and BTAS_ASSERT_POLICY is not, it sets the default (ON -> BTAS_ASSERT_THROW, OFF -> BTAS_ASSERT_ABORT) and issues a DEPRECATION message. BTAS_ASSERT_THROWS=1 is still defined as a macro when the policy is BTAS_ASSERT_THROW, and btas/error.h still honors a user-defined BTAS_ASSERT_THROWS when BTAS_ASSERT_POLICY is not defined — so older consumers keep working unchanged.

BTAS_ASSERT no longer supplies the trailing semicolon of a statement (it is a do-while now, which also fixes the dangling-else hazard the THROW form had), so the handful of call sites in btas/generic/ that relied on it got their semicolons.

Tests

unittest/assert_policy_test.cc is compiled 3 times, once per policy and always with NDEBUG defined, and run as btas/unit/run/assert/{THROW,ABORT,IGNORE} (picked up by the check-btas target). Each run verifies the mode: the exception is thrown and the argument was evaluated exactly once, the process aborts (SIGABRT handler), or nothing happens and the argument was not evaluated.

$ ctest -R assert
    Start 3: btas/unit/build/assert/THROW    ... Passed
    Start 4: btas/unit/run/assert/THROW      ... Passed
    Start 5: btas/unit/build/assert/ABORT    ... Passed
    Start 6: btas/unit/run/assert/ABORT      ... Passed
    Start 7: btas/unit/build/assert/IGNORE   ... Passed
    Start 8: btas/unit/run/assert/IGNORE     ... Passed
100% tests passed, 0 tests failed out of 6

Configure matrix checked (macOS, clang, Ninja): default (BUILD_TESTING=ON) -> THROW; BUILD_TESTING=OFF -> ABORT; BUILD_TESTING=OFF -DCMAKE_BUILD_TYPE=Release -> IGNORE; -DBTAS_ASSERT_THROWS=ON/OFF -> THROW/ABORT + deprecation message; -DBTAS_ASSERT_POLICY=BOGUS -> fatal error. The exported INTERFACE_COMPILE_DEFINITIONS carries the policy in each case. The BTAS headers that use BTAS_ASSERT were syntax-checked under all 3 policies.

The Catch-based unit tests, which catch BTAS_ASSERT failures, are now skipped with a warning when the policy is not BTAS_ASSERT_THROW, instead of failing to compile via the #error in unittest/test.h.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Unresolved moderate findings remain in CMake defaults, policy-test configuration, and the SIGABRT handler.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds explicit BTAS_ASSERT THROW, ABORT, and IGNORE policies with CMake propagation, compatibility handling, documentation, and dedicated tests.

Changes:

• Implements and exports the three assertion policies.
• Updates assertion call sites and compatibility behavior.
• Adds policy-specific tests and documentation.

File summaries

File	Summary	Review notes
unittest/test.h	Restricts Catch tests to THROW mode.	None
unittest/CMakeLists.txt	Adds policy-specific test targets.	Moderate (1 vote): policy tests are skipped when BUILD_TESTING=OFF.
unittest/assert_policy_test.cc	Verifies all three assertion modes.	Moderate (2 votes): the SIGABRT handler uses non-async-signal-safe functions.
README.md	Documents policy options and defaults.	None
CMakeLists.txt	Configures, validates, and exports the assertion policy.	Moderate (2 votes): default handling misses RelWithDebInfo and multi-config Release/MinSizeRel cases.
btas/generic/tuck_cp_als.h	Updates assertion statements.	None
btas/generic/element_wise_contract.h	Updates assertion statements.	None
btas/generic/cp_als.h	Updates assertion statements.	None
btas/error.h	Implements assertion policy behavior and diagnostics.	Nit (1 vote): the non-IGNORE branch label is inverted.

Review details

Suppressed comments (2)

btas/error.h:88

• The #else branch below is the THROW/ABORT implementation, so this condition label is inverted and will mislead anyone maintaining the policy split. Please label it as the non-IGNORE branch.

#else // BTAS_ASSERT_POLICY == BTAS_ASSERT_IGNORE

unittest/CMakeLists.txt:44

• This loop is only configured when the parent CMakeLists.txt enters its if (BUILD_TESTING) branch, so with BUILD_TESTING=OFF none of the three policy executables or tests exists. That contradicts the comment that these checks run regardless of configuration and leaves the no-testing configurations without the policy coverage introduced here; move the policy-test setup outside that guard or update the intended coverage.

# BTAS_ASSERT policy checks: btas/error.h is compiled against each of the 3
# policies (with NDEBUG defined, to confirm that none of them depends on it),
# regardless of how this build was configured
foreach(_policy THROW ABORT IGNORE)
  set(_exec btas_assert_policy_${_policy}_test)
  add_executable(${_exec} EXCLUDE_FROM_ALL assert_policy_test.cc)

• Files reviewed: 9/9 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

Resolve the multi-config build-type handling and stale BTAS_ASSERT_THROWS cache default issues.

Review details

Suppressed comments (2)

CMakeLists.txt:87

• On a multi-config generator, CMAKE_BUILD_TYPE is not guaranteed to be empty; a project that passes -DCMAKE_BUILD_TYPE=Release still leaves this variable set even though Xcode/Visual Studio ignore it. This branch then selects BTAS_ASSERT_IGNORE for every configuration and skips the multi-config warning, so a Debug build can silently compile with assertions disabled. Check GENERATOR_IS_MULTI_CONFIG before the build-type test (after the BUILD_TESTING case) so the documented global multi-config default remains BTAS_ASSERT_ABORT.

elseif (CMAKE_BUILD_TYPE STREQUAL Release OR CMAKE_BUILD_TYPE STREQUAL MinSizeRel)
  set(BTAS_ASSERT_POLICY_DEFAULT BTAS_ASSERT_IGNORE)

CMakeLists.txt:76

• On an in-place upgrade, BTAS_ASSERT_THROWS remains in CMakeCache.txt from the previous redefaultable_option (including its default OFF). Because this branch checks only DEFINED, a prior BUILD_TESTING=OFF Release/MinSizeRel build is treated as an explicit deprecated override and selects BTAS_ASSERT_ABORT instead of the documented BTAS_ASSERT_IGNORE default. Please handle or document stale cache entries so the new defaults are not changed unintentionally.

if (DEFINED BTAS_ASSERT_THROWS AND NOT DEFINED BTAS_ASSERT_POLICY)

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Lite