Correct ~36 comments that describe behaviour the framework no longer has - #508
Merged
Merged
Conversation
A twelve-way audit of all 64 headers under include/morph (28,496 lines) found that the dominant comment defect in this tree is not verbosity -- it is comments that are simply false. This corrects the ones verified against the code, plus the ten places where a spec carries the same wrong claim. Every fix below was checked by reading the code it describes; none changes behaviour. core/bridge.hpp - `registrationInFlight` was documented as set only on the async path and only after `registerModelAsync` returns `true`. It is set unconditionally, before the call, on every path including the synchronous fallback -- the assignment's own comment says so, 1500 lines away. - `BridgeLifetime` claimed "the sole guarded region is `~BridgeHandler`'s `deregisterHandler` call". PR #491 added two more; the paragraph's non-blocking argument covers only the first. - Two sites called `executeVia`'s backend read a "lock-free snapshot". `loadBackend()` takes `_backendMtx`; only the `currentId` read is lock-free. docs/spec/core/bridge.md already had this right. - `liveness()` said `~BridgeHandler` checks it. It uses the `BridgeLifetime` gate -- which is the whole point of morph#486, and the distinction a reader at that comment most needs. - "five other sites read under `_attachMtx`" -- there are eight. Replaced with a form that cannot drift again. core/remote.hpp - The shutdown gate rejects `attach` too, and has since PR #20. Two comments said "register and execute envelopes only", which misdescribes graceful shutdown for the shared-instance topology. - `finding 035` resolves to nothing. examples/FINDINGS.md retired that citation form and warns it has already silently resolved to the wrong finding once; this was the last surviving instance in include/. - `closeConnection` promised a later `execute` gets "model not found". True only for a private instance -- a shared one with a surviving attacher is not erased. - `setLogProvider` said "every `register` envelope"; an `attach` that misses the directory constructs the instance and consults it too. - `handleInline` said "only safe for `register`, `deregister`". It rejects only `execute`; `SimulatedRemoteBackend` routes six kinds through it. - Two direction words pointing the wrong way ("comparisons below" now in detail/reply_router.hpp; "the members above" is 1250 lines down). core/wire.hpp, model_key.hpp, timeout_scheduler.hpp, strand.hpp, registry.hpp - `RemoteServer::execute` does not exist (it is `dispatchExecute`). - `makeErr`'s rationale predates `EscapingWriteOpts`: JSON validity is now guaranteed for every field, so what this call still does is output sanitization -- `sanitizeControlChars`'s own doc already says so. - `BRIDGE_KEY_FROM` was credited twice with specialising `ModelKeyTraits`; it emits only `ActionKeyTraits`. `BRIDGE_MODEL_KEY` is the one meant. - `Bridge::_liveness` was removed in bbe6051. - `scheduleNext` deliberately does *not* use `scoped_lock` over both mutexes; the comment citing that spelling contradicts the comment explaining why. - The `WireSchemasUnsatisfiable` fixture is in tests/test_wire_schemas.cpp. - Two dead `docs/planned/` paths (the content shipped under docs/spec/). forms/forms.hpp - "closes the two gaps glaze leaves open" is followed by twelve bullets. - "glaze's schema writer emits no `required` array at all" is false for the vendored glaze, and the file's own code overwrites `required` precisely because it may have emitted one. - `x-decimalPlaces` was described as the unit's default in two places; it is the field's *declared* precision, as a third site and the spec both say. - `kExactDoubleLimit`'s brief said 2^53 is the largest value a double holds exactly. It is the largest N with every integer in [0, N] representable. (The comparisons against it are all correct.) - A cross-reference to an unshipped plan document. util/rational.hpp - `scaleFactorsFor`'s @return had `leftScaled` and `rightScaled` exactly backwards relative to all four call sites -- the one comment an overflow audit would most trust. - `reportClamp` pointed at a `catch` morph#158 deleted. - The `fromFloat` bound comment said INT64_MIN "is a valid result"; canonicalise clamps it to -INT64_MAX with an error log. offline/, journal/, qt/, render/, net/ - `ReconnectOutcome::Reconnected` promised replay; `onOnline()` re-polls `shouldContinue()` and may skip it -- the spec has a section titled after exactly this. - `QueueItem::id`: both durable queues re-present the stored id, not a fresh one. - `stepOrThrow` said a busy/error code "is treated the same as reaching the end"; it throws. - `FileOfflineQueueError`'s doc claimed the open failures, which throw plain `std::runtime_error`. - `SyncWorker::stop()` documented as one-shot; a stop landing mid-run costs the *next* run too. - `EscapingWriteOpts` was duplicated "so this header stays free of a `core/` dependency" -- it includes three core/ headers. - `LogEntry` said application code never constructs one; outbox rows are exactly that, as three other comments in the same file say. - `registerModelAsync`'s "@return `true` always" is false whenever `asyncRegistrationEnabled` is unset, which is the default. - `SlotRegistry` ships; it was cited as planned, via a dead path. - Two "Task 6" references pointing at an unrelated Kanban GUI plan task. Specs corrected where they mirror the same claim: core/backend.md (shutdown gate, x2), core/bridge.md (registrationInFlight), core/shared_instances.md (said "There is no `assignPrimaryAsync`" -- there is, and Bridge prefers it), util/rational.md (described a canonicalise that no longer exists, and warned about a fixed hazard while the real one is undocumented), offline/offline.md (x3), journal/journal.md (x2), forms/forms.md. Also re-pins the twelve branch-coverage allowlist entries the added and removed comment lines moved; all 25 verified to land on their own `source` text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
CI caught what my local check did not. The Windows clang-cl presets build with
`-Weverything -Werror`, which includes `-Wdocumentation`; the Linux legs and my
own `-fsyntax-only` do not, so this only ever surfaced on Windows:
file_offline_queue.hpp(35,56): error: empty paragraph passed to '@throws'
command [-Werror,-Wdocumentation]
35 | /// type -- see the constructor's own `@throws`.
Backticks do not stop clang parsing `@throws` in prose as a Doxygen command.
Reworded. It was the only such instance in the branch, and all edited headers
now pass `-Wdocumentation -Werror` locally, with glaze as a system header the
way CI treats it.
Two corrections to my own previous commit, both found by re-verifying each new
comment rather than trusting the audit that prompted it:
- `bridge.hpp` said contextKey/primary are read under `_attachMtx` at "five
other sites". That count was wrong, so I replaced it with "every other site"
-- which is also wrong: `registerHandlerImpl` reads `binding->contextKey` at
two sites (:1760, :1814) holding neither `_attachMtx` nor `_mtx`, against an
invariant the `_attachMtx` member comment states as absolute. Now says "the
other attach/assign sites" and names the exception, which is filed as
morph#505 rather than papered over.
- `forms.hpp` said glaze "emits a `required` array only for a type that declares
`meta<V>::required`". A tagged variant's discriminator also gets one
(glaze/json/schema.hpp:840-843). The load-bearing half -- that glaze never
*derives* `required` from member types, which is why morph writes its own --
is unaffected; the absolute was too strong. Corrected in the header and in
docs/spec/forms/forms.md.
Also re-pins the branch-coverage allowlist entries the reworded lines moved.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
All six verified against the code before acting; four are cases where correcting a stale comment introduced a new inaccuracy, which is the specific risk of a change like this one. - `render/i18n.hpp` -- the rewrite turned a stale prediction into a false present-tense claim: `morph::render` does not "share" `SlotRegistry`, which is a QML type in module `MorphForms` and in no C++ namespace at all. Now says so, and says not to look for it under this namespace. - `forms.hpp` / `docs/spec/forms/forms.md` -- "glaze never *derives* `required` from member types" is false for the pinned glaze: `glz::requires_key` (glaze/core/common.hpp:744, called per member at json/schema.hpp:976) returns true when `Opts.error_on_missing_keys` is set and the member is not nullable. Nothing is derived under the options `schemaJson<A>()` actually uses, which is the load-bearing part -- so state that, rather than an unconditional property of glaze that would mislead anyone who later sets that option. - `remote.hpp` -- `handleInline`'s new enumeration listed six kinds and said "all six". `SimulatedRemoteBackend::deregisterModel` (:1945) routes `makeDeregister` through it too, so it is seven, and `deregister` was the one omitted -- in a comment a reader would consult precisely to ask whether `deregister` may be dispatched inline. - `model_key.hpp` -- fixing the macro name broke the description. `BRIDGE_MODEL_KEY` does not "name the type"; it deduces it (`MemberTypeOf<decltype(MEMBER)>`, :305). As written it also contradicted the next sentence, which only makes sense if the non-alias route infers. - `docs/spec/journal/journal.md` -- the new paragraph sits under "IActionLog -- the storage interface" but qualified the append-only rule with `rotate()` and `repairTornTail()`. Neither is on `IActionLog` (its virtuals are `append`, `flush`, `entries`); both are `FileActionLog`'s, and `repairTornTail()` is private and constructor-only, so no caller can invoke it. An implementor over a database sink owes neither. - `scripts/branch_partial_allowlist.json` -- the sharpest one: the PR re-pinned the `line` fields but left three `reason` prose cross-references citing the old numbers, which is exactly the stale-line-reference class this PR exists to eliminate. Fixed, and audited the whole file both ways -- every `source` pin lands on its own text and every "line N"/".hpp:N" in prose resolves to a real entry. Also re-pins the three entries the `remote.hpp` fix above shifted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
The PR description listed "four 'added in a later task' markers" and "two dead docs/planned/ paths" among what this branch fixed. It had not fixed them. Rather than weaken the claim, make it true -- they are the same class as everything else here and each referent was verified present: - forms.hpp x4 -- `Greater`/`GreaterOrEqual`/`Less`/`LessOrEqual` (:830-:995), `ExactlyOneOf`/`AtLeastOneOf`/`MutuallyExclusive` (:1233-:1327), `Equals` (:1073) and `VisibleWhen`/`ReadonlyWhen` (:1377/:1431) are all defined in this file. None is future work, and one of the four also cited "the planned spec" for a worked example that ships in docs/spec/forms/forms.md. - app.hpp -- `morph::views::ViewTraits<V>` exists (forms/views.hpp:402), so the stated precondition for a ViewScreen counterpart is met; the comment read as blocked work when nothing blocks it, and named a docs/planned/ file that does not exist. - bridge.hpp -- the last docs/planned/shared_model_instances.md reference; the content ships as docs/spec/core/shared_instances.md. `grep -rn "docs/planned\|later task" include/` is now empty. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Replaces #503, which GitHub auto-closed when its base branch (#492) was deleted on merge. Same branch, same commits, now rebased onto master. Full review history is on #503.
Output of a twelve-way audit over all 64 headers in
include/morph(28,496 lines).What the audit actually found
The brief asked for correctness and "comments up to date and compressed enough". The dominant defect is not verbosity — this codebase's long why comments are house style and carry real reasoning, and I briefed the auditors explicitly not to flag length. It is comments that are false: roughly 60 assert behaviour the code no longer has. This PR fixes the ~36 verified by reading the code they describe, plus the spec passages carrying the same wrong claim.
Doxygen is clean: four audit agents ran it with the repo's exact
WARN_AS_ERROR=FAIL_ON_WARNINGSsettings, validating their harness against a planted gap first.Highlights
scaleFactorsFor's@returnwas exactly backwards —leftScaled/rightScaledswapped relative to all four call sites. The one comment an overflow audit would most trust.liveness()said~BridgeHandlerchecks it. It uses theBridgeLifetimegate — that distinction is morph#486.attachtoo, since PR Stateful models: keyed shareable instances, instance subscriptions, and a bank example that demonstrates them (closes #18) #20; two comments said "register and execute only", misdescribing graceful shutdown for the shared-instance topology.finding 035resolves to nothing.examples/FINDINGS.mdretired that citation form and records that it already resolved to the wrong finding once. Last surviving instance ininclude/.BridgeLifetime's "sole guarded region" — Batch: forms boundaries, bridge lifetime sites 1-3, replay-ledger promotion, scheduled mutation gate #491 added two more, and the paragraph's non-blocking argument covers only the first.core/shared_instances.mdsaid "There is noassignPrimaryAsync." There is.util/rational.mdwarned about a hazard that was fixed while the real one, one file away, was undocumented — now pointed at util: formatRationalDecimal negates INT64_MIN in int64 (UB), under a comment claiming it widens first #496.grep -rn "docs/planned\|later task" include/is now empty — four "added in a later task" markers for code defined in the same file, and the last three deaddocs/planned/paths.Corrections made during review
Fixing false comments risks replacing them with differently-false ones. Three self-review rounds plus
/code-review highcaught nine such cases, all fixed here rather than merged:bridge.hpp— I replaced a wrong count ("five other sites") with a wrong universal ("every other site");registerHandlerImplreadscontextKeyat two sites holding neither lock. Now names the exception; defect filed as bridge: registerHandlerImpl reads binding->contextKey under neither lock, violating the invariant _attachMtx's own comment states #505.forms.hpp— "glaze never derivesrequiredfrom member types" is false:glz::requires_keyderives one whenOpts.error_on_missing_keysis set.remote.hpp— the newhandleInlinelist said "all six";SimulatedRemoteBackend::deregisterModelmakes it seven, andderegisterwas the omitted one.model_key.hpp— fixing the macro name broke the description:BRIDGE_MODEL_KEYdeduces the key type, it does not name it.render/i18n.hpp— turned a stale prediction into a false present-tense claim;SlotRegistryis a QML type in no C++ namespace.journal.md— qualifiedIActionLog's rule with twoFileActionLogmembers, one private and constructor-only.file_offline_queue.hpp— a literal`@throws`in prose broke the Windows build under-Weverything -Werror; backticks do not stop clang parsing it as a Doxygen command.branch_partial_allowlist.json—linefields were re-pinned but threereasoncross-references still cited the old numbers: this PR's own subject.Scope
Comments and documentation only — no behavioural change. Where a comment was wrong because the code is wrong, the defect is filed and left alone: #493–#502, #505, #506, #507.
Verification
clang++ -std=c++23 -fsyntax-onlyand-Wdocumentation -Werror, with glaze as a system header, matching CI.scripts/check_spec_citations.shclean.scripts/branch_partial_allowlist.jsonaudited both ways: everysourcepin lands on its own text, and everyline N/.hpp:Ninsidereasonprose resolves to a real entry. 25/25.Remaining stale comments and the cross-cutting duplication (the "public macro surface" paragraph appears 17-18 times) are tracked in #504.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv