An invalid (default-constructed / moved-from) expression_holder is rejected by some entry points and silently accepted by others:
| Entry point |
Behaviour on an invalid holder |
holder.get(), *holder, holder-> |
throws "expression_holder: access to invalid (null) expression" |
invalid + x (operators) |
throws (same message) |
to_string(invalid) |
no throw, returns a string |
scalar_evaluator::apply(invalid) |
no throw, returns 0 (scalar_evaluator.h:29-36) |
diff(invalid, x) |
no throw, returns a valid holder |
So ev.apply(std::move(e)) after an accidental move, or evaluating a holder that a failed rebuild left empty, returns 0.0 with no signal — a silent wrong value, the class the rest of this week's work has been removing. diff producing a valid result from nothing is the same problem one step later.
Severity: low–medium. Needs a programming error to reach, but the failure is silent and the inconsistency means callers cannot rely on either behaviour.
Fix: pick one contract and apply it everywhere. Given get() already throws, the consistent choice is for apply, diff, substitute, to_string/to_latex and the limit/contains entry points to call throw_if_invalid() (or get()) up front. The evaluators' is_valid() branches that return ValueType{0} should become throws; the differentiation visitors' internal "invalid = zero accumulator" convention is separate and can stay internal.
Test: one parameterised test that feeds a default holder to each public entry point and expects invalid_expression_error (or whichever type is chosen).
Signed-off-by: petlenz [email protected]
An invalid (default-constructed / moved-from)
expression_holderis rejected by some entry points and silently accepted by others:holder.get(),*holder,holder->"expression_holder: access to invalid (null) expression"invalid + x(operators)to_string(invalid)scalar_evaluator::apply(invalid)scalar_evaluator.h:29-36)diff(invalid, x)So
ev.apply(std::move(e))after an accidental move, or evaluating a holder that a failed rebuild left empty, returns0.0with no signal — a silent wrong value, the class the rest of this week's work has been removing.diffproducing a valid result from nothing is the same problem one step later.Severity: low–medium. Needs a programming error to reach, but the failure is silent and the inconsistency means callers cannot rely on either behaviour.
Fix: pick one contract and apply it everywhere. Given
get()already throws, the consistent choice is forapply,diff,substitute,to_string/to_latexand the limit/contains entry points to callthrow_if_invalid()(orget()) up front. The evaluators'is_valid()branches that returnValueType{0}should become throws; the differentiation visitors' internal "invalid = zero accumulator" convention is separate and can stay internal.Test: one parameterised test that feeds a default holder to each public entry point and expects
invalid_expression_error(or whichever type is chosen).Signed-off-by: petlenz [email protected]