fix(service-registry): make the convenience registerService honor its documented assertion - #169
Merged
Merged
Conversation
… documented assertion The non-returning overloads claimed "Asserts in debug builds" but discarded the tryRegisterService status in every configuration, so a duplicate or null registration silently altered the service surface a plugin sees. A rejection now trips PJ_ASSERT, the doc-comments state the exact NDEBUG behavior, and the builder's registration rules gain their first regression tests (built with PJ_ASSERT_THROWS so the invariant is observable under RelWithDebInfo). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BqLATym6kKna4SHhjYXnkh
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
1 participant
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.
Summary
ServiceRegistryBuilder::registerService(both convenience overloads) documented "Asserts in debug builds" while the body did(void)status— no assertion existed in any configurationPJ_ASSERTwith a clear message; the doc-comments state the exact NDEBUG behavior (status silently dropped) and direct callers that must report failures totryRegisterServiceservice_registry_builder_test— the first regression coverage for the builder's registration rules (first-registration success, duplicate-keeps-first, null ctx/vtable rejection, and the assert behavior viaPJ_ASSERT_THROWS, which keeps the invariant observable under RelWithDebInfo whereassert()compiles away)Companion
PlotJuggler/PJ4#546 switches every PJ4 host call site to a status-checked path with visible diagnostics — that PR delivers production visibility and is independent of this one (it uses the existing
tryRegisterService); this PR closes the doc/code mismatch and guards debug/CI builds.Validated in the PJ4 superbuild at the pinned submodule (330/330 tests, 6/6 in the new target); the diff was transplanted onto current main with a 3-way apply (clean) — CI here validates it against HEAD.
🤖 Generated with Claude Code
https://claude.ai/code/session_01BqLATym6kKna4SHhjYXnkh