Fix any() assertion combinator to correctly resolve OR semantics#598
Merged
Conversation
any() was incorrectly combining sub-assertion results: the last sub-argument's truth value silently overrode the accumulated OR result, because CRI_ASSERT_SPECIFIER_ANY_INDIRECT spliced the sub-assertion's raw expansion directly into the accumulator instead of assigning it to its own temporary first (unlike the sibling ALL_INDIRECT/NONE_INDIRECT macros), and CRI_ASSERT_SPECIFIER_any combined the final result with the last leaf's residual value using && instead of the combinator's own || operator. As a result, any(eq(int, 1, 1), eq(int, 0, 2), eq(int, 0, 3)) incorrectly failed even though the first sub-assertion is true, matching the exact shape reported in Snaipe#595. Fixes Snaipe#595
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
As reported in #595, the
any()assertion combinator does not actuallycombine its sub-assertions with OR semantics. Instead, the result silently
collapses to the truth value of the last sub-argument:
Root cause
Two separate issues in
include/criterion/internal/new_asserts.hcombine to produce this behavior:
CRI_ASSERT_SPECIFIER_ANY_INDIRECTsplices each sub-assertion's rawexpansion directly into
Cond = Cond || <expansion>, instead of firstassigning it to its own temporary (
cri_cond_un) like the siblingALL_INDIRECT/NONE_INDIRECTmacros do.CRI_ASSERT_SPECIFIER_anycombines the final accumulated result with*cri_pass = *cri_pass && cri_cond;— using&&instead of thecombinator's own
||operator. Forall()/none()this iscoincidentally harmless (AND-absorption with 0 holds for an AND-chain),
but for
any()the combinator can be correctly true even when the lastevaluated leaf is false — exactly the reported scenario — so ANDing the
correct
cri_condwith a stale residual wrongly forces the result to 0.Both edits are needed together; applying only the first is not sufficient
to fix #595 (verified by testing).
Fix
CRI_ASSERT_SPECIFIER_ANY_INDIRECT: assign the sub-assertion result tocri_cond_unfirst, thenCond = Cond || cri_cond_un(mirrorsALL_INDIRECT).CRI_ASSERT_SPECIFIER_any: change*cri_pass = *cri_pass && cri_cond;to
*cri_pass = *cri_pass || cri_cond;(mirrors howall()/none()each combine using their own operator).
Testing
Verified against the real, unmodified header by compiling the macro
expansion with a minimal stub runtime (the small set of
cri_assert_node_*/criterion_*_testsupport functions), since the fullmeson build wasn't viable in the sandbox. Before the fix, both
any(1==1, 2==2, 0==3)andany(0==1, 2==2, 0==3)(the exact shapes from#595) incorrectly reported failure. After the fix, both correctly pass,
and
all()/none()sanity cases are unaffected (no regression).Fixes #595