Make disabled EBGEOMETRY_EXPECT truly vanish (unevaluated sizeof)

[rmrsk]

Summary

Background

EBGEOMETRY_EXPECT is opt-in: with EBGEOMETRY_ENABLE_ASSERTIONS undefined (the default, and the
release/release-test presets) it expanded to ((void)(cond)). That still evaluates the
condition — the discard only removes the branch, not the work — so eliminating it was left to the
optimiser, which is not guaranteed. Any condition the compiler cannot prove side-effect-free
survives into the generated code; the std::isfinite() preconditions in the SoA distance kernels
are the sharp case, since they run on the order of 20× per nearest-neighbor query. At -O0 with
assertions off, the whole set survives.

Solution

Expand the disabled branch to (static_cast<void>(sizeof((cond)))) instead. sizeof is an
unevaluated operand, so the condition is still parsed — it stays syntax-checked, and variables or
parameters that appear only inside assertions still count as used — but is never executed, at any
optimisation level. Disabled assertions now have exactly zero runtime cost and cannot have
observable side effects.

The change is ported from the EBGeometry checkout used by chombo-discharge, where an interleaved A/B
of its NearestNeighbor benchmark measured PointCloudBVH's uniform-cube query ~4–5 % faster with
the expressions truly gone. That benchmark suite is not part of this repository, so the figure is
quoted rather than reproduced here.

Docs/Sphinx/source/ConfigurationOptions.rst (Sec:Assertions) and the macro's Doxygen @details
block are updated in the same commit; no other page describes the expansion, so no further .rst
work was needed.

Side-effects

Assertion conditions are no longer evaluated in assertions-OFF builds. Two constructs that were
previously legal in a condition now become compile errors: a void-typed expression (no size), and a
lambda-expression (ill-formed in an unevaluated operand in C++17). All 826 EBGEOMETRY_EXPECT call
sites under Source/ were audited — none uses either, none has side effects in its condition, and
none spans multiple lines.

The regression risk worth watching in CI is unused-entity suppression, since the -Werror paths
build with assertions off: Tests/CMakeLists.txt (-Wall -Wextra -Werror, including the
release-test preset) and the Intel icpx job (-Wunused-variable -Wunused-but-set-variable -Werror over the Examples). Locally, GCC 13.3 treats a sizeof appearance as a use for both
warnings, and every Examples/*/main.cpp compiles clean under that exact flag set with assertions
off. Clang's -Wunused-but-set-variable and icpx are only reachable in CI. clang-tidy is
unaffected — Scripts/clang-tidy-check.sh configures the debug preset, where assertions are on
and this branch is never compiled.

Local verification: debug, debug-san, release-test and examples suites all pass (290/290 on
release-test), and Scripts/run-all-checks.sh completes clean end to end, including clang-tidy,
Doxygen and Sphinx.

Alternative solutions

• Keep ((void)(cond)) and rely on the optimiser — rejected: not guaranteed for conditions with
  opaque calls, and untrue at -O0.
• Expand to nothing at all — rejected: the condition would stop being syntax-checked in
  assertions-off builds (letting a broken assertion land unnoticed), and assertion-only variables
  and parameters would start tripping unused-entity warnings under -Werror.

Reviewer checklist (to be completed by a human)

• The test suite compiles and runs to completion without warnings or errors.
• All relevant new features are documented in the user documentation (Sphinx).
• This contribution does not break existing sections in the user documentation.
• All relevant APIs are documented in the doxygen documentation.
• Appropriate labels have been assigned to this PR.
• New or revised proper licensing and copyright information is in place.
• A PR review has been run using @claude review.
• The continuous integration and testing hooks at GitHub run to completion.

🤖 Generated with Claude Code

[rmrsk]

@claude review

[github-actions]

Claude finished @rmrsk's task in 1m 47s —— View job

---

Review complete

• Gather context (diff, files changed)
• Review Source/EBGeometry_Macros.hpp changes
• Review Docs/Sphinx/source/ConfigurationOptions.rst changes
• Check for related sites (other macros, docs cross-references, call-site audit)
• Post review feedback

Summary

This is a small, well-scoped, and well-justified change. Switching the disabled EBGEOMETRY_EXPECT expansion from ((void)(cond)) to (static_cast<void>(sizeof((cond)))) (Source/EBGeometry_Macros.hpp:67) correctly moves the condition into an unevaluated operand — it keeps the "still parsed, so still syntax-checked and no unused-entity warnings" property of the old macro while guaranteeing zero runtime evaluation at any optimization level, not just when the optimizer manages to prove the branch dead. That's a real, useful guarantee (-O0 debug/sanitizer builds previously did pay for every EBGEOMETRY_EXPECT condition).

A few things I checked and confirmed look right:

• Double parens sizeof((cond)): correct and necessary — without the extra parens, if cond happened to look like a type-id, sizeof would try to parse it as one; wrapping forces expression parsing.
• Comma safety: I grepped Source/ for EBGEOMETRY_EXPECT call sites containing a top-level comma (which would break macro argument splitting) — all matches (e.g. EBGeometry_TriangleSoAImplem.hpp:1224-1226, dot(v21, v21) > T(0)) have the comma safely nested inside inner parens, so this isn't a new hazard.
• Call-site count: grep -rn EBGEOMETRY_EXPECT Source/*.hpp | wc -l → 826, matching the PR body's audit figure.
• Docs: Docs/Sphinx/source/ConfigurationOptions.rst (Sec:Assertions) is updated consistently with the new expansion and rationale; I checked the other .rst files that mention EBGEOMETRY_EXPECT (Building.rst, TestingLocally.rst, Implementation.rst, ContributionGuidelines.rst) and none of them describe the old expansion's internals, so no further doc drift.
• Doxygen @details in EBGeometry_Macros.hpp:27-33 is accurate and matches the new behavior.

Minor, non-blocking observations

• EBGeometry_Macros.hpp:65-66 (the inline // comment right above the #define) largely restates the Doxygen @details block a few lines above (EBGeometry_Macros.hpp:27-33). Not wrong, just a little redundant — purely a style nit, not worth a revision on its own.
• The two newly-introduced breaking cases called out in the PR body (a void-typed condition, or a lambda expression as the condition) are real and correctly identified as ill-formed inside sizeof's unevaluated operand in C++17. Worth keeping in mind for any future EBGEOMETRY_EXPECT call site that a contributor might add, but the stated audit of the existing 826 sites covers today's code.

I did not find any correctness issues. The stated local verification (all four CMake presets + run-all-checks.sh) is exactly the right coverage for this kind of change; I wasn't able to independently re-run the build in this review environment, but nothing in the diff itself raises a concern that testing wouldn't already have caught.