Conversation
…62) material_ref::get() was guarded by an assert alone, and the Release configuration is -O3 -DNDEBUG, so the only thing standing between a use-before-wire and a null dereference compiled away in exactly the builds that run real analyses. It now throws, naming the target. Not the finalize-time sweep the issue proposed: wire_materials() already rejects a missing or wrongly-typed target, so after it succeeds every ref is wired by construction and a sweep would be unreachable and untestable. The reachable case is a read BEFORE finalize -- a compute callback invoked by hand, a constructor that resolves too eagerly -- which got no diagnosis at all. The cost is one correctly-predicted branch on a path the performance review measured as a plain pointer dereference and not a hotspot. material_interface's virtual update() is gone. It was vestigial from before the property engine: nothing ever called it. Work happens only through callbacks bound to a property, by passing a member pointer to add_output() or by assigning traits().update directly. But the obvious way to write a material is void update() override { m_out = ...; } which compiled, ran nothing, and left the property at its initial value with no diagnostic -- documented in one inline comment, in weighted_sum.h. Removing the virtual makes that a compile error on the `override`, which is the spelling a newcomer reaches for. It found one immediately: a test material in test_drucker_prager.cpp had exactly that shape. The materials that define update() keep it -- it is their callback, passed to add_output as a member pointer, which never needed a virtual. Both solvers that call this->update() bind it themselves and are unaffected. 305/305. Mutation tested: removing either get() overload's check fails AMaterialRefReadBeforeFinalizeThrows, and restoring the virtual fails the static_assert. The first version of the ref test reached only the mutable overload, so deleting the const one's check left the suite green -- it now drives both, before and after finalize.
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.
Closes #59, #62.
material_ref::get()was assert-only (#59)The Release configuration is
-O3 -DNDEBUG, so the only guard between a use-before-wire and a null dereference compiled away in exactly the builds that run real analyses. It now throws, naming the target.Not the finalize-time sweep the issue proposed.
wire_materials()already rejects a missing or wrongly-typed target, so after it succeeds every ref is wired by construction -- a sweep there would be unreachable and untestable. The reachable case is a read before finalize: a compute callback invoked by hand, a constructor that resolves too eagerly. That got no diagnosis at all.Cost is one correctly-predicted branch, on a path the performance review measured as a plain pointer dereference and explicitly not a hotspot.
The vestigial
virtual update()is gone (#62)Nothing ever called it. Work happens only through callbacks bound to a property -- a member pointer passed to
add_output(), ortraits().updateassigned directly. But the obvious way to write a material iswhich compiled, ran nothing, and left the property at its initial value with no diagnostic. It was documented in exactly one place: an inline comment in
weighted_sum.h.Removing the virtual makes that a compile error on the
override, which is the spelling a newcomer reaches for. It found one immediately -- a test material intest_drucker_prager.cpphad exactly that shape.Materials that define
update()keep it: it is their callback, passed as a member pointer, which never needed a virtual. Both solvers that callthis->update()bind it themselves and are unaffected.Tests
tests/test_wiring_contract.cpp, 5 tests. 305/305.const get()check removedAMaterialRefReadBeforeFinalizeThrowsT& get()check removedAMaterialRefReadBeforeFinalizeThrows,TheUnwiredRefErrorNamesTheTargetupdate()restoredstatic_assertinMaterialInterfaceHasNoUpdateToOverrideThe first version of the ref test was worthless and mutation testing is the only reason I know. It called
get()only through a non-const path, so deleting the check from theconstoverload left the entire suite green. It now drives both overloads, before and afterfinalize().Because #62's failure mode is a compile error in someone else's material, nothing here would otherwise notice the virtual coming back -- hence the
static_assertguard, plus a test that a material's ownupdate()still drives its property throughadd_output, so removing the virtual did not remove the mechanism.