diff --git a/CMakeLists.txt b/CMakeLists.txt index dc2deed981..9280f10401 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -140,6 +140,25 @@ if(${CMAKE_SYSTEM_NAME} MATCHES "Linux") set(CMAKE_CXX_FLAGS_RELEASE "-O2 -DNO_DEBUG") endif() +# ############################################################################## +# Asserts +# ############################################################################## + +# CAF_ASSERT follows the semantics of the standard assert(): active in Debug, +# compiled out in optimized builds (Release, RelWithDebInfo, MinSizeRel). Enable +# this option to keep the asserts active in an optimized build as well, which is +# useful when reproducing a problem in a RelWithDebInfo build with a debugger +# attached. +option(RESINSIGHT_ENABLE_ASSERTS_IN_RELEASE + "Keep CAF_ASSERT active in optimized builds (Release, RelWithDebInfo)" + OFF +) +mark_as_advanced(RESINSIGHT_ENABLE_ASSERTS_IN_RELEASE) +if(RESINSIGHT_ENABLE_ASSERTS_IN_RELEASE) + message(STATUS "Enabling CAF_ASSERT in optimized builds") + add_definitions(-DCAF_ENABLE_ASSERTS=1) +endif() + # ############################################################################## # OpenMP # ############################################################################## diff --git a/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafAssert.h b/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafAssert.h index 3add52c1e0..3b8a199d90 100644 --- a/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafAssert.h +++ b/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafAssert.h @@ -4,12 +4,48 @@ #include #include -#define CAF_ASSERT( expr ) \ - do \ - { \ - if ( !( expr ) ) /* NOLINT */ \ - { \ - std::printf( "%s : %i : CAF_ASSERT( %s ) failed\n", __FILE__, __LINE__, #expr ); \ - std::abort(); \ - } \ +// CAF_ASSERT documents an invariant that must hold if the program is correct. It is meant for +// programming errors, not for run-time conditions that can legitimately occur (bad input files, +// missing data, failed network calls) - those need real error handling. +// +// Following the semantics of the standard assert(), CAF_ASSERT is active in debug builds and +// compiled out in optimized builds (NDEBUG). A failing assert prints file/line/expression and +// calls std::abort(), which raises SIGABRT and is picked up by the crash handler installed in +// RiaMain.cpp, so the failure is written to the log file and reported to OpenTelemetry with a +// stack trace. +// +// Developers who want the asserts to stay active in an optimized build (typically RelWithDebInfo) +// can configure with -DRESINSIGHT_ENABLE_ASSERTS_IN_RELEASE=ON, which defines CAF_ENABLE_ASSERTS=1 +// for the whole build. +// +// CAF_ENABLE_ASSERTS is given a default here so this header stays self-contained: include order can +// never silently switch the asserts off. See OPM/ResInsight#14397 for the class of bug this avoids. +#ifndef CAF_ENABLE_ASSERTS +#ifdef NDEBUG +#define CAF_ENABLE_ASSERTS 0 +#else +#define CAF_ENABLE_ASSERTS 1 +#endif +#endif + +#if CAF_ENABLE_ASSERTS == 1 + +#define CAF_ASSERT( expr ) \ + do \ + { \ + if ( !( expr ) ) /* NOLINT */ \ + { \ + std::fprintf( stderr, "%s : %i : CAF_ASSERT( %s ) failed\n", __FILE__, __LINE__, #expr ); \ + std::abort(); \ + } \ } while ( false ) + +#else + +// The expression is kept inside an unevaluated sizeof rather than discarded outright. It is not +// evaluated, so there is no run-time cost and no side effects, but it is still type checked (the +// assert cannot bit-rot into something that no longer compiles) and any variable used only by the +// assert still counts as referenced, which avoids a wave of unused-variable warnings. +#define CAF_ASSERT( expr ) ( (void)sizeof( !( expr ) ) ) /* NOLINT */ + +#endif diff --git a/docs/agents/coding-style.md b/docs/agents/coding-style.md index 6f7760939c..781c4ce47b 100644 --- a/docs/agents/coding-style.md +++ b/docs/agents/coding-style.md @@ -112,6 +112,36 @@ RiaLogging::info( QString( "Exported %1 cells for well '%2'." ).arg( count ).arg RiaLogging::info( std::format( "Exported {} cells for well '{}'.", count, wellName ) ); ``` +### Asserts + +Use `CAF_ASSERT` (`cafAssert.h`). Do not add new `CVF_ASSERT`, `CVF_ASSERT_MSG`, `CVF_FAIL_MSG` or `CVF_TIGHT_ASSERT` — those are legacy and are being migrated away from. + +An assert documents an invariant that holds whenever the program is correct. It is not error handling. Conditions that can legitimately occur at run time — a missing file, a malformed input deck, a failed network call, a user selecting nothing — must be handled with a return value, `std::optional`, `std::expected` or a logged error message, never with an assert. + +```cpp +// Bad – a missing file is a run-time condition, not a broken invariant +CAF_ASSERT( QFile::exists( fileName ) ); + +// Good – handled and reported +if ( !QFile::exists( fileName ) ) +{ + RiaLogging::error( std::format( "File not found: {}", fileName ) ); + return {}; +} + +// Good – a broken invariant, unreachable unless the code is wrong +CAF_ASSERT( index >= 0 && index < m_values.size() ); +``` + +`CAF_ASSERT` follows the semantics of the standard `assert()`: + +- **Debug builds**: active. A failure prints file, line and expression, then calls `std::abort()`. +- **Optimized builds** (Release, RelWithDebInfo): compiled out. The expression is still type checked but never evaluated, so it must be free of side effects — never put work inside the assert that the surrounding code depends on. + +Configure with `-DRESINSIGHT_ENABLE_ASSERTS_IN_RELEASE=ON` to keep the asserts active in an optimized build, which is useful when reproducing a problem in RelWithDebInfo with a debugger attached. + +A failing assert aborts via `SIGABRT`, which the crash handler installed in `RiaMain.cpp` picks up: the failure is written to the log file and reported to OpenTelemetry with a stack trace. + ### Lambda Functions Keep lambdas short and readable: