From 96b0ff6a21f7bcbdad3897b442a547f39d6ab2e4 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 23 Sep 2026 17:39:20 +0200 Subject: [PATCH 1/5] agents: comments and docs carry current state and reasoning, never history The codebase had drifted to 52% comment lines in `include/morph` -- 18,581 of 35,732 -- with single unbroken blocks running to 145, 124, 92 and 88 lines, and 1,946 issue references spread across code and `docs/`. A large part of that is history written into the wrong medium. A comment saying what the code used to do, or naming the ticket that changed it, duplicates something `git blame` already holds exactly and permanently -- except the copy rots at the next edit, and it sends the reader out of the file to a tracker to understand the line in front of them. So the rule is now explicit: a comment, and a page under `docs/`, states what the code does now and why. Reasoning stays and is the point -- "why this and not the obvious alternative" is a current fact about a current constraint, not history. What goes is the narrative of how the code arrived here. Public API documentation is exempt from brevity and not from the rule: Doxygen runs with WARN_AS_ERROR, so every public symbol keeps complete @param/@tparam/@return -- written fully, and without a ticket number in them. Length is left to follow from the rules rather than being capped. Once the history and the citations are gone, most long blocks are short, and a limit would only invite padding up to it or truncating reasoning that earns its space. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- AGENTS.md | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index c03c692f..eee1d865 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -73,6 +73,49 @@ running the `triage-issue` skill (`.claude/skills/triage-issue/`). An author labelling their own issue `triage: valid` records only that they thought it worth filing, which every open issue already implies. +## Comments and documentation + +A comment, and a page under `docs/`, states **what the code does now and why**. +Nothing else. + +- **No history.** Not what the code used to do, not what was tried and + abandoned, not what a change replaced. `git blame` and `git log` hold that + accurately and permanently; a comment holds a copy that starts rotting the + moment the next change lands. Reach for blame when you need the story. +- **No issue numbers, no commit hashes.** A reader should not have to leave the + file — or reach a tracker — to understand the line in front of them. If a + ticket's reasoning is worth keeping, keep the *reasoning*, in the reader's + own words, at the place it applies. +- **Reasoning stays, and is the point.** "Why this and not the obvious + alternative", "this bound is what the hardware gives us", "this order matters + because the lock is held" — none of that is history. It is a current fact + about a current constraint, and it is the most valuable thing a comment + carries. + +Length follows from the rules rather than from a limit: once the history and the +citations are gone, a block that ran for eighty lines is usually five. + +The one exception is **public API documentation**, which is exempt from brevity +and not from the rules above. Doxygen runs with `WARN_AS_ERROR = +FAIL_ON_WARNINGS`, so every public symbol keeps complete +`@param`/`@tparam`/`@return` — write those fully, and still without a ticket +number in them. + +A worked contrast: + +```cpp +// BAD — history, a citation, and a reader sent elsewhere +// Until morph#604 this used notify_one, which lost a wakeup when two +// waiters blocked on different predicates (see also morph#489's comment). +// Restored to notify_all in a1b2c3d. +_cv.notify_all(); + +// GOOD — the current constraint, in place +// notify_all, not notify_one: waiters block on different predicates, so a +// single wakeup can land on one whose predicate is still false and be lost. +_cv.notify_all(); +``` + ## Verify rather than assert A control that reports success while measuring nothing is the failure mode this From e5ef07bc117b7e1c84cc7e6153fa6420210df69b Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 23 Sep 2026 17:51:54 +0200 Subject: [PATCH 2/5] comments(qt+render+session+journal): state the constraint, drop the history Comments in these four subsystems carried the story of how they got here alongside what they do: ticket citations, "before this ...", "exactly as before", "(today's behavior)" on every default, and in one case a whole `@par` devoted to what an earlier version of that same paragraph had said. Every sentence was tested against "does this describe the code as it is now, or how it got here?". What it describes now stays, including the long blocks -- `locale_format.hpp`'s measured Unicode locale facts, the UTF-8 strictness argument in `decodeUtf8`, `qt_executor.hpp`'s teardown hazard -- because all of that is live constraint rather than narrative. What was left is the reasoning, in place, with no reader sent to a tracker. No code changed; this is comment text only. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- include/morph/journal/action_log.hpp | 4 +- include/morph/journal/action_log_json.hpp | 12 +- include/morph/journal/file_action_log.hpp | 14 +- include/morph/journal/journal.hpp | 27 ++- include/morph/journal/outbox.hpp | 12 +- .../morph/qt/forms/forms_controller_core.hpp | 31 ++- include/morph/qt/qt_executor.hpp | 22 +- include/morph/qt/qt_websocket_backend.hpp | 52 +++-- include/morph/qt/qt_websocket_server.hpp | 31 ++- include/morph/render/locale_format.hpp | 203 ++++++++---------- include/morph/session/session.hpp | 4 +- include/morph/session/session_auth.hpp | 2 +- 12 files changed, 185 insertions(+), 229 deletions(-) diff --git a/include/morph/journal/action_log.hpp b/include/morph/journal/action_log.hpp index c9936e2f..77494552 100644 --- a/include/morph/journal/action_log.hpp +++ b/include/morph/journal/action_log.hpp @@ -141,8 +141,8 @@ struct LogEntry { /// /// Declared here rather than beside the codec in `action_log_json.hpp`: a /// caller catching it needs only ``, and making that catch drag in -/// glaze would put the surcharge back on exactly the consumers this split -/// exists to spare (morph#573, step 4). +/// glaze would put the compile surcharge back on exactly the consumers the +/// split exists to spare. struct SerializationError : std::runtime_error { using std::runtime_error::runtime_error; }; diff --git a/include/morph/journal/action_log_json.hpp b/include/morph/journal/action_log_json.hpp index 30e36148..94b77323 100644 --- a/include/morph/journal/action_log_json.hpp +++ b/include/morph/journal/action_log_json.hpp @@ -11,12 +11,12 @@ /// @brief The `LogEntry` JSON codec, kept apart from `action_log.hpp`. /// /// `action_log.hpp` defines `LogEntry`, `Outcome`, `IActionLog` and the -/// process-wide log slot; `core/model.hpp` includes it for `IActionLog` alone, -/// and every consumer that reaches a model therefore used to compile -/// `` whether or not it ever serialised anything. That is the -/// cliff morph#521 measured and morph#573 step 4 names: `action_log.hpp` was -/// 252,559 preprocessed lines and 2.67 CPU-s against `core/strand.hpp`'s -/// 127,217 and 1.20. +/// process-wide log slot; `core/model.hpp` includes it for `IActionLog` alone. +/// Were the codec declared there too, every consumer that reaches a model would +/// compile `` whether or not it ever serialised anything, and +/// that is a measured cliff rather than a tidiness point: with glaze included, +/// `action_log.hpp` is 252,559 preprocessed lines and 2.67 CPU-s, against +/// `core/strand.hpp`'s 127,217 and 1.20. /// /// Include this header instead when you need `toJson`/`fromJson`. Inside /// morph, exactly one header does: `file_action_log.hpp`. diff --git a/include/morph/journal/file_action_log.hpp b/include/morph/journal/file_action_log.hpp index 1a46e7f6..568fc731 100644 --- a/include/morph/journal/file_action_log.hpp +++ b/include/morph/journal/file_action_log.hpp @@ -107,8 +107,8 @@ class FileActionLog : public IActionLog { throw std::runtime_error("FileActionLog: failed to open " + _path.string()); } // "a" mode creates the file if it did not already exist -- a fresh - // directory entry that `_file`'s own later fsyncs never make durable - // (morph#532). Unconditional: harmless when the file already existed, + // directory entry that `_file`'s own later fsyncs never make durable. + // Unconditional: harmless when the file already existed, // since syncing an unchanged directory is a cheap no-op: a failure // here is surfaced rather than swallowed, the same discipline // `flush()`/`rotate()` already apply to the file-content fsync. @@ -183,7 +183,7 @@ class FileActionLog : public IActionLog { line.push_back('\n'); long long const offsetBeforeWrite = ::morph::core::wideFtell(_file); if (_io.fwrite(line.data(), line.size(), _file) != line.size()) { - // See FileOfflineQueue::writeLine's identical comment (morph#530): + // See FileOfflineQueue::writeLine's identical comment: // "a"-mode means a short write's partial bytes sit exactly where the // next append() would resume, merging into one line repairTornTail() // can only heal at construction, before this can happen. Roll the @@ -374,7 +374,7 @@ class FileActionLog : public IActionLog { // Two directory mutations just happened -- the seal rename and, on // success, a brand-new active file -- and neither is durable until - // its directory entry is fsynced (morph#532). Run regardless of what + // its directory entry is fsynced. Run regardless of what // failed above, so a rotation that is about to throw still leaves // whatever succeeded as durable as it can be made; the failure is // surfaced below rather than swallowed, same as the pre-rotation @@ -383,9 +383,9 @@ class FileActionLog : public IActionLog { // constructor's own directory fsync -- but it is not silent either: // the contract is that it warns, so an operator knows the rotated // names are only as durable as the filesystem makes them. Collapsing - // the tri-state to a bool here used to drop that warning on the floor - // for both directories, leaving `rotate()` quieter than the - // construction path that documents the same classification. + // the tri-state to a bool here would drop that warning on the floor for + // both directories, leaving `rotate()` quieter than the construction + // path that documents the same classification. auto const classifyAndWarn = [this](const std::filesystem::path& dir) { auto const outcome = ::morph::core::classifyDirectorySync(_io.syncPath(dir)); if (outcome == ::morph::core::DirectorySync::unsupported) { diff --git a/include/morph/journal/journal.hpp b/include/morph/journal/journal.hpp index cdd0789c..8be9d5d9 100644 --- a/include/morph/journal/journal.hpp +++ b/include/morph/journal/journal.hpp @@ -25,10 +25,10 @@ namespace morph::journal { /// (`LogEntry::schema`) disagrees with the fingerprint this build /// computes for the same action, and no migration covers the pair. /// -/// This is the signal the journal previously did not have. Before it, the same -/// situation produced a successful reconstruction of a state that was never -/// recorded — a renamed field decoding to its default, reported with as much -/// confidence as a correct one. There is no "degraded" reconstruction to fall +/// Without this signal the same situation reconstructs, successfully, a state +/// that was never recorded — a renamed field decoding to its default, reported +/// with as much confidence as a correct one. There is no "degraded" +/// reconstruction to fall /// back to and no way to tell how much of the entry survived the decode, so /// this throws rather than warning: an audit trail that cannot be reconstructed /// faithfully must say so. @@ -61,7 +61,7 @@ struct SchemaMismatchError : std::runtime_error { /// *unverifiable*: there is no record of the shape that produced its payload, /// so no check can be performed at all. enum class UnstampedPayloadPolicy : std::uint8_t { - /// @brief Replay it, exactly as every morph build before this check did. + /// @brief Replay it, without verifying a fingerprint it does not carry. /// /// The default, because it is the only choice that keeps journals written /// by earlier builds replayable at all — refusing them by default would @@ -142,11 +142,11 @@ class PayloadMigrationRegistry { private: using Key = std::pair; - // Both functors, not just the hash. This map named `PairKeyHash` alone for - // as long as it has existed, and `unordered_map` enables heterogeneous - // lookup only when the hash *and* the equality are transparent -- so every - // `find` built a `Key` to probe with, silently, while looking like it did - // not (morph#699). `add` still builds one, because it inserts. + // Both functors, not just the hash: `unordered_map` enables heterogeneous + // lookup only when the hash *and* the equality are transparent. Name the + // hash alone and every `find` silently builds a `Key` to probe with, while + // looking like it does not. `add` builds one regardless, because it + // inserts. std::unordered_map _migrations; }; @@ -242,8 +242,8 @@ class ScopedReplayFlag { /// - **Different, with a migration registered** for `(actionType, entry.schema)` /// in @p migrations — the migration rewrites the payload JSON in memory, and /// the rewritten bytes are dispatched. The stored entry is untouched. -/// - **Different, with no migration** — throw `SchemaMismatchError`. This is the -/// case that used to reconstruct a state nobody ever recorded. +/// - **Different, with no migration** — throw `SchemaMismatchError`. Decoding +/// it anyway is what would reconstruct a state nobody ever recorded. /// - **Entry unstamped** (`schema` empty) — governed by @p unstamped; see /// `UnstampedPayloadPolicy`. /// @@ -286,8 +286,7 @@ inline std::unique_ptr<::morph::model::detail::IModelHolder> replay( // re-dispatching it would very likely throw the same exception again // (the same rejected precondition), aborting reconstruction outright. // Skipping it is exactly "replay only committed facts", which is what - // this function already promised before Failed entries could appear in - // the same log stream (issue #23). + // this function promises. if (entry.outcome == Outcome::Failed) { continue; } diff --git a/include/morph/journal/outbox.hpp b/include/morph/journal/outbox.hpp index c6f5a8db..7151c7bb 100644 --- a/include/morph/journal/outbox.hpp +++ b/include/morph/journal/outbox.hpp @@ -18,8 +18,7 @@ namespace morph::journal { /// A null `drainOutbox`/`markRelayed` already throws a catchable /// `std::bad_function_call` (invoking a null `std::function`); this makes a /// null `sink` consistent with that instead of a raw null-`shared_ptr` -/// dereference (real undefined behavior, not portably catchable — see -/// `LASTRADA-Software/morph#95`). +/// dereference, which is real undefined behavior and not portably catchable. struct NullSinkError : std::runtime_error { using std::runtime_error::runtime_error; }; @@ -134,11 +133,10 @@ struct OutboxRelay { ::morph::log::logError("[journal::OutboxRelay] null sink"); } // This branch itself is unit-tested (test_outbox.cpp asserts the - // warning fires). relay() itself throws NullSinkError right after - // this call if sink is null and there is at least one row to relay - // (see LASTRADA-Software/morph#95) -- a null drainOutbox/markRelayed - // still throws std::bad_function_call as usual, invoking a null - // std::function. + // warning fires). relay() throws NullSinkError right after this call + // if sink is null and there is at least one row to relay -- a null + // drainOutbox/markRelayed still throws std::bad_function_call as + // usual, invoking a null std::function. } }; diff --git a/include/morph/qt/forms/forms_controller_core.hpp b/include/morph/qt/forms/forms_controller_core.hpp index 8ff468e1..658f834b 100644 --- a/include/morph/qt/forms/forms_controller_core.hpp +++ b/include/morph/qt/forms/forms_controller_core.hpp @@ -4,21 +4,20 @@ /// @file /// Model-agnostic core of the shipped Qt/QML forms renderer's controller: -/// owns (or composes over) the Bridge/BridgeHandler/executor wiring -/// `examples/forms/gui_qml`'s `FormsController` used to hardcode per-app, and +/// owns (or composes over) the Bridge/BridgeHandler/executor wiring, and /// exposes the two operations `DynamicForm.qml` needs -- submit and /// options-fetch -- generically over `BridgeHandler::executeJson`, so -/// an app depends on this directly instead of re-deriving the wiring. A -/// concrete `QObject`/`QML_ELEMENT` wrapper per app (Qt cannot register a +/// an app depends on this directly instead of re-deriving the wiring per app. +/// A concrete `QObject`/`QML_ELEMENT` wrapper per app (Qt cannot register a /// class *template* for QML) forwards to this core and turns its callbacks /// into signals -- see `examples/forms/gui_qml/FormsController.hpp` for the /// reference wrapper. /// /// Two constructor overloads decide who owns the `Bridge`: /// - The single-argument (schema-only) constructor builds and owns a private -/// `ThreadPoolExecutor` + `QtExecutor` + `Bridge` over a `LocalBackend`, -/// exactly as before -- the convenient default for a demo or an app that -/// has no `Bridge` of its own. +/// `ThreadPoolExecutor` + `QtExecutor` + `Bridge` over a `LocalBackend` -- +/// the convenient default for a demo or an app that has no `Bridge` of its +/// own. /// - The `(Bridge&, IExecutor*, schemasJson)` constructor composes over a /// caller-supplied `Bridge`/executor instead -- the caller decides the /// deployment mode (`LocalBackend`, `SimulatedRemoteBackend`, @@ -101,11 +100,10 @@ class FormsControllerCore { /// @brief Executes @p optionsAction with @p bodyJson to fetch a `Choice` /// field's combo-box options, via the same generic `executeJson` - /// path `submitIfValid` uses -- @p optionsAction is never - /// hardcoded, unlike the pre-factoring example controller, and - /// @p bodyJson is a true pass-through (not always `"{}"`), so a - /// dependent `Choice` (`x-optionsDependsOn`) can send - /// `{parentField: value, ...}` instead of an empty body. + /// path `submitIfValid` uses. @p optionsAction is a parameter rather + /// than a hardcoded id, and @p bodyJson is a true pass-through (not + /// always `"{}"`), so a dependent `Choice` (`x-optionsDependsOn`) + /// can send `{parentField: value, ...}` instead of an empty body. /// @tparam OnReply Callable invoked with the options-action result JSON on success. /// @tparam OnError Callable invoked with the `std::exception_ptr` on failure. /// @param optionsAction Registered action type id that serves the options. @@ -123,14 +121,11 @@ class FormsControllerCore { private: /// @brief The private pool/executor/backend bundle the schema-only - /// constructor builds and owns, exactly as `FormsControllerCore` - /// always did before the `(Bridge&, IExecutor*, ...)` overload - /// existed. Absent (`_owned` unengaged) when the core instead - /// composes over a caller-supplied `Bridge`/executor. + /// constructor builds and owns. Absent (`_owned` unengaged) when the + /// core instead composes over a caller-supplied `Bridge`/executor. /// /// Declaration order within the struct matters for destruction: `bridge` - /// must tear down before `pool`/`gui`, exactly as the pre-factoring - /// `FormsController` required. + /// must tear down before `pool`/`gui`, so it is declared last. struct OwnedBridge { morph::exec::ThreadPoolExecutor pool{2}; ::morph::qt::QtExecutor gui; diff --git a/include/morph/qt/qt_executor.hpp b/include/morph/qt/qt_executor.hpp index d830fad5..502fd43c 100644 --- a/include/morph/qt/qt_executor.hpp +++ b/include/morph/qt/qt_executor.hpp @@ -27,13 +27,13 @@ namespace morph::qt { /// *delivered*. Without a guard, an event still on the Qt queue at teardown is /// delivered against a freed executor and reads `_context` off freed memory. /// -/// That is not hypothetical. `Bridge::executeVia` chains three `Completion` -/// objects per dispatched action, each settled from inside the previous one's -/// delivered callback, so a caller waiting only on its own top-level completion -/// can observe "done" while an intermediate post is still queued. When that -/// stale event is finally pumped, its body calls `post()` for the next link in -/// the chain — against an executor that no longer exists. It segfaults on an -/// ordinary uninstrumented build, not only under a sanitizer. +/// `Bridge::executeVia` reaches that state on an ordinary run: it chains three +/// `Completion` objects per dispatched action, each settled from inside the +/// previous one's delivered callback, so a caller waiting only on its own +/// top-level completion can observe "done" while an intermediate post is still +/// queued. When that stale event is pumped, its body calls `post()` for the +/// next link in the chain — against a freed executor. It segfaults on an +/// uninstrumented build, not only under a sanitizer. /// /// Each queued task therefore carries a weak observer of this executor's /// lifetime and does nothing if the executor is already gone. Dropping is the @@ -44,8 +44,8 @@ namespace morph::qt { /// **Boundary of the guarantee.** The check assumes this executor is destroyed /// on the same thread that runs its context's event loop, which holds for every /// owner in this repository. Destroying one from another thread while its loop -/// is mid-delivery still needs external synchronisation: this closes the "torn -/// down with events still queued" hole, not a genuine cross-thread race. +/// is mid-delivery still needs external synchronisation: the token covers +/// teardown with events still queued, not a genuine cross-thread race. class QtExecutor : public ::morph::exec::IExecutor { public: /// @brief Constructs an executor that posts tasks to @p context's thread. @@ -54,8 +54,8 @@ class QtExecutor : public ::morph::exec::IExecutor { /// tasks; `QMetaObject::invokeMethod` dispatches to whichever thread /// `context->thread()` reports at the time each task is posted, so tasks /// posted after `context` is moved to a different thread run there. - /// Defaults to `QCoreApplication::instance()`, preserving the previous - /// GUI-thread-only behaviour. Passing `nullptr` (e.g. when constructed + /// Defaults to `QCoreApplication::instance()`, i.e. the GUI thread. + /// Passing `nullptr` (e.g. when constructed /// before `QCoreApplication` exists) makes `post()` a no-op, matching /// `QMetaObject::invokeMethod`'s own handling of a null target. Borrowed, /// not owned: a non-null @p context must outlive this executor. diff --git a/include/morph/qt/qt_websocket_backend.hpp b/include/morph/qt/qt_websocket_backend.hpp index 95556924..702f3217 100644 --- a/include/morph/qt/qt_websocket_backend.hpp +++ b/include/morph/qt/qt_websocket_backend.hpp @@ -43,10 +43,9 @@ struct QtWebSocketBackendConfig { /// /// Defaults to `false`: both settle their `Completion` from inside the call, /// having blocked the Qt thread in a nested `QEventLoop` for the round trip - /// — `IBackend`'s own default behaviour, and what every existing embedder - /// (a desktop Qt client, this backend's own test suite) already relies on, - /// since it makes a handler usable on the line after `BridgeHandler`'s - /// constructor returns. + /// — `IBackend`'s own default behaviour, and what a desktop Qt embedder + /// relies on, since it makes a handler usable on the line after + /// `BridgeHandler`'s constructor returns. /// /// Set `true` for a build where that blocking call cannot happen at all — /// a WASM main thread, where Qt refuses to spin a nested loop and the @@ -58,9 +57,9 @@ struct QtWebSocketBackendConfig { /// with "handler not bound" for an unbound binding rather than queuing or /// blocking. /// - /// This flag chooses *whether the transport blocks*. It is no longer an - /// opt-in to a second set of interface verbs: the continuation exists on - /// both paths, because `bindModel` returns a `Completion` either way. + /// This flag chooses *whether the transport blocks*, nothing more: the + /// continuation exists on both paths, because `bindModel` returns a + /// `Completion` either way. bool asyncRegistrationEnabled = false; }; @@ -224,19 +223,18 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// so `contextKey` is the *only* channel by which the instance's identity /// reaches it. `RemoteServer::attachLogIfConfigured` returns without /// consulting its `LogProvider` at all when the envelope's `contextKey` is - /// empty, so dropping it here did not merely lose an entity key — it left - /// the instance **unjournalled** (morph#594). `SimulatedRemoteBackend` and - /// `morph::net::SocketBackend` (morph#587) override this for the same - /// reason; backends documented as interchangeable must not disagree about - /// whether a private registration is audited. + /// empty, so dropping the key here would not merely lose an entity key — it + /// would leave the instance **unjournalled**. `SimulatedRemoteBackend` and + /// `morph::net::SocketBackend` override this for the same reason; backends + /// documented as interchangeable must not disagree about whether a private + /// registration is audited. /// /// This is also the verb the *blocking* `bindModel` path reaches for an /// empty-`primary`, zero-`current` request, and the one /// `Bridge::switchBackend` calls directly when it re-registers a handler - /// after a reconnect — so before morph#594 the key was dropped whatever - /// `Config::asyncRegistrationEnabled` was set to on a backend swap, and - /// dropped on every private registration when it was unset. `bindModel`'s - /// own non-blocking path already carried it. + /// after a reconnect — so it carries the key on a backend swap and on every + /// private registration, whatever `Config::asyncRegistrationEnabled` is set + /// to. /// /// `registerModel` forwards here with an empty key, so there is one place /// that builds this envelope rather than two that can drift apart. @@ -309,9 +307,8 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// set, because that is exactly when a completion is settled by /// `onTextMessage` — a Qt slot, delivered by the event loop of the thread /// that called `bindModel`. A caller blocked in a wait is not running that - /// event loop, so the reply it is waiting for can never arrive: the - /// deadlock morph#568 exists to remove, which on a WASM main thread aborts - /// the page outright. + /// event loop, so the reply it is waiting for can never arrive — a deadlock, + /// which on a WASM main thread aborts the page outright. /// /// With the flag unset this backend's `bindModel` is `IBackend`'s default, /// which settles inside the call, so `kCallerMayBlock` is both true and @@ -321,7 +318,7 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// Note which way round this reads. It does not say "registration is /// asynchronous" — `SocketBackend`'s is too, and it answers /// `kCallerMayBlock` because a separate I/O thread settles its completions. - /// It says only that *this* thread must not stop and wait. See morph#593. + /// It says only that *this* thread must not stop and wait. /// /// @return `kCallerMustNotBlock` when `Config::asyncRegistrationEnabled` is /// set, `kCallerMayBlock` otherwise. @@ -373,9 +370,8 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// Unlike `bindModel`, this does **not** consult /// `Config::asyncRegistrationEnabled`: promotion happens from inside the /// result `Completion`'s callback chain, where no caller is left blocked - /// waiting for it either way, so there is no synchronous guarantee to - /// preserve — which is why the optional non-blocking promote this - /// replaces (removed by morph#571) had no opt-in gate either. + /// waiting for it either way, so there is no synchronous guarantee an + /// opt-in gate would protect. /// /// The documented no-op cases (empty `primary`, zero `mid`) resolve with /// @p request's `mid` without sending anything, matching @@ -403,7 +399,7 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// model registered on the server indefinitely. /// /// Assigned a real, non-zero `callId` from the same counter/namespace - /// `execute()`/`bindModel()` use (see issue #65): `callId == 0` + /// `execute()`/`bindModel()` use: `callId == 0` /// is reserved for a parked synchronous control call's reply, and a /// fire-and-forget `deregister` sharing that sentinel could otherwise have /// its own stray "ok" reply handed to an unrelated `registerModel`'s @@ -494,7 +490,7 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// @return `true` if a pending control call was found and settled. bool tryRouteControlReply(const ::morph::wire::Envelope& env); - /// @brief Drops the reply to a fire-and-forget deregister (issue #65). + /// @brief Drops the reply to a fire-and-forget deregister. /// @param env Decoded reply envelope. /// @return `true` if the id belonged to a pending deregister. bool tryRouteDeregisterReply(const ::morph::wire::Envelope& env); @@ -562,8 +558,8 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// /// One map for both verbs, not two: `register`, `registerShared`, `attach` /// and `assign` replies are all matched identically — a bare `modelId` - /// echoed against the `callId` — so the split the four `*Async` verbs used - /// to justify has nothing left to represent. + /// echoed against the `callId` — so a per-verb split would have nothing to + /// represent. std::unordered_map::Promise> _pendingRegistrations; @@ -580,7 +576,7 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { std::vector _queuedRegistrations; /// @brief Call-ids of `deregister` envelopes still awaiting their (unused) - /// reply (see issue #65). + /// reply. /// /// `deregisterModel` is fire-and-forget: nobody observes the reply, but it /// still needs a real, non-zero `callId` so `onTextMessage` can recognise diff --git a/include/morph/qt/qt_websocket_server.hpp b/include/morph/qt/qt_websocket_server.hpp index 6edb2c3f..44c5bac1 100644 --- a/include/morph/qt/qt_websocket_server.hpp +++ b/include/morph/qt/qt_websocket_server.hpp @@ -25,7 +25,7 @@ namespace morph::qt { /// is evaluated (same rationale as `morph::qt::QtWebSocketBackendConfig` and /// `morph::offline::NetworkMonitorConfig`). struct QtWebSocketServerConfig { - /// @brief Max simultaneous live client connections. `0` = unbounded (today's behavior). + /// @brief Max simultaneous live client connections. `0` = unbounded. /// /// A new connection beyond this count is closed immediately in `onNewConnection`, /// before any message exchange and before it is tracked internally. @@ -33,10 +33,9 @@ struct QtWebSocketServerConfig { /// @brief Per-frame size cap enforced before a message reaches `RemoteServer::handle()`. /// - /// Defaults to `morph::wire::kMaxEnvelopeBytes` (the wire layer's own bound), so - /// an unconfigured server behaves exactly as today: the wire-layer cap is the - /// only one in effect. Set lower to reject oversized frames earlier, before the - /// cost of a pool round-trip and JSON decode. + /// Defaults to `morph::wire::kMaxEnvelopeBytes`, the wire layer's own bound, so an + /// unconfigured server is capped only there. Set lower to reject oversized frames + /// earlier, before the cost of a pool round-trip and JSON decode. std::size_t maxMessageBytes = ::morph::wire::kMaxEnvelopeBytes; /// @brief Per-connection token-bucket rate limit, in messages per second. `0` = unbounded. @@ -52,7 +51,7 @@ struct QtWebSocketServerConfig { std::size_t messagesPerSecond = 0; /// @brief Time allowed for a newly-accepted connection to send its first text - /// frame before it is closed. `0` = disabled (today's behavior). + /// frame before it is closed. `0` = disabled. /// /// `QWebSocketServer::newConnection()` fires only after the WebSocket (and, in /// `SecureMode`, TLS) opening handshake has already completed, so in practice @@ -61,14 +60,13 @@ struct QtWebSocketServerConfig { std::chrono::milliseconds handshakeTimeout{0}; /// @brief Time a connection may go without sending any frame before it is - /// closed. `0` = disabled (today's behavior). + /// closed. `0` = disabled. /// /// Checked by a periodic housekeeping sweep (roughly once per second), so the /// actual close can lag the configured value by up to that sweep interval. std::chrono::milliseconds idleTimeout{0}; - /// @brief Address `listen()` binds to. Default `QHostAddress::LocalHost` - /// (today's behavior, unchanged). + /// @brief Address `listen()` binds to. Default `QHostAddress::LocalHost`. QHostAddress bindAddress = QHostAddress::LocalHost; /// @brief Deliberate opt-out of the exposure guard: set `true` only to @@ -94,8 +92,7 @@ struct QtWebSocketServerConfig { /// @par Resource limits /// Pass a `QtWebSocketServerConfig` to bound connection count, per-frame size, /// per-connection message rate, and handshake/idle time. All fields default to -/// unbounded (except `maxMessageBytes`, which defaults to the wire-layer cap), -/// reproducing today's behavior when omitted. +/// unbounded, except `maxMessageBytes`, which defaults to the wire-layer cap. /// /// @par Bind address & plaintext-exposure guard /// `listen()` refuses — returns `false` and logs at `morph::log::LogLevel::error` @@ -123,8 +120,7 @@ class QtWebSocketServer : public QObject { /// declared at all on an SSL-less Qt build (`QT_NO_SSL`) — see the /// class doc comment's "SSL-less Qt builds" section. /// @param cfg Per-connection resource limits. Default: everything unbounded - /// (today's behavior) except `maxMessageBytes`, which defaults to - /// the wire-layer cap. + /// except `maxMessageBytes`, which defaults to the wire-layer cap. /// @param parent Optional Qt parent object. explicit QtWebSocketServer(::morph::backend::RemoteServer& server, quint16 port = 0, #ifndef QT_NO_SSL @@ -172,14 +168,13 @@ class QtWebSocketServer : public QObject { /// `RemoteServer::drainedWithin()` to report every in-flight execute has /// replied, sending each still-connected client a real close frame /// (`CloseCodeGoingAway`, reason `"server shutting down"`) instead of an - /// abort, and finally running the existing `close()` hard stop for - /// whatever @p deadline did not leave time to finish gracefully. + /// abort, and finally running the `close()` hard stop for whatever + /// @p deadline did not leave time to finish gracefully. /// /// Pumps the Qt event loop internally while it waits, so it is safe to /// call from the Qt thread — which is also the thread that must run /// `sendTextMessage` for replies and close frames to actually reach - /// clients. Purely additive and opt-in: a server that never calls this - /// behaves exactly as it does today, and `close()` itself is unchanged. + /// clients. /// /// @param deadline Total time budget for the whole sequence, measured /// from the moment this call starts. Whatever the drain @@ -208,7 +203,7 @@ class QtWebSocketServer : public QObject { /// @brief Current token-bucket balance for `messagesPerSecond`. double tokens = 0.0; - /// @brief Last time `tokens` was refilled (used to compute elapsed time on the next frame). + /// @brief Last time `tokens` was refilled; the next frame's refill is computed from it. std::chrono::steady_clock::time_point lastRefill; /// @brief Last time any frame was received on this connection (drives `idleTimeout`). diff --git a/include/morph/render/locale_format.hpp b/include/morph/render/locale_format.hpp index e2c67d49..a43cf52b 100644 --- a/include/morph/render/locale_format.hpp +++ b/include/morph/render/locale_format.hpp @@ -13,24 +13,17 @@ /// `.`-decimal text), and a renderer calls `normalizeLocaleNumber` once, at /// the point text leaves the control, before handing it to those routines. /// -/// @par One aggregate, not a row of swappable views (morph#591) -/// Both functions take a single `NumericLocale`. They used to take the locale -/// facts as four and five positional `std::string_view`s, every one of which -/// was silently swappable with its neighbours -- the header carried a -/// clang-tidy suppression block for `bugprone-easily-swappable-parameters`, -/// with a paragraph of justification, on each of the two functions and on the -/// sign helper below. (Spelling the marker out here would suppress nothing and -/// trip `clang-tidy-nolint`'s unmatched-begin check, which is why this -/// paragraph names the check instead.) Adding a digit base would have made -/// six adjacent views. With the aggregate a call site -/// names each fact (`{.decimalSeparator = ",", .groupSeparator = "."}`), no two -/// parameters of either function share a type, and all three suppressions are -/// deleted rather than widened. The next locale fact -- a percent sign, an -/// exponent separator -- is then a new defaulted member rather than a seventh +/// @par One aggregate, not a row of swappable views +/// Both functions take a single `NumericLocale` rather than a row of positional +/// `std::string_view`s. Six adjacent views of the same type are silently +/// swappable with each other, which is what `bugprone-easily-swappable-parameters` +/// exists to catch; with the aggregate a call site names each fact +/// (`{.decimalSeparator = ",", .groupSeparator = "."}`) and no two parameters of +/// either function share a type. The next locale fact -- a percent sign, an +/// exponent separator -- is a new defaulted member rather than a seventh /// parameter. The two edges taking the *same* type is the point as much as the -/// naming is: "these two must agree" becomes structural instead of a convention -/// a caller can get half right, which is the drift morph#591 and morph#599 were -/// both about. +/// naming is: "these two must agree" is structural instead of a convention a +/// caller can get half right. /// /// @par Separators are strings, not characters /// The locale facts are `std::string_view`, because a real locale's separator @@ -39,8 +32,7 @@ /// as `char`, those cannot be expressed at all: the caller can only pass some /// single byte that never matches, so a perfectly valid `"1 050,25"` typed by a /// French user normalises to `std::nullopt` and the entry is reported -/// malformed. An empty view means "this locale has no such separator" (the role -/// `'\0'` used to play). +/// malformed. An empty view means "this locale has no such separator". /// /// @par So is the negative sign /// For the same reason, and measured rather than assumed: of the 711 locales @@ -49,13 +41,13 @@ /// control -- U+061C ARABIC LETTER MARK, U+200E LEFT-TO-RIGHT MARK or U+200F /// RIGHT-TO-LEFT MARK -- making it two or three code points, and ar_DZ does so /// even though its sign is the ordinary hyphen. Matched as a single `char`, -/// none of those round-trips: the display edge emitted a sign the entry edge -/// then rejected. So `negativeSign` is matched as a whole string too, -/// defaulting to `"-"` so that every existing caller is unchanged (morph#583). +/// none of those round-trips as a single `char`: the display edge would emit a +/// sign the entry edge then rejected. So `negativeSign` is matched as a whole +/// string too, defaulting to the ASCII `"-"`. /// /// @par And so is the positive sign, on the entry edge only /// `normalizeLocaleNumber` reads `NumericLocale::positiveSign` and *drops* what -/// it matches, because canonical text has no `'+'` in it (morph#596). +/// it matches, because canonical text has no `'+'` in it. /// `formatCanonicalNumber` never emits one: a positive number displays unsigned /// in every locale, and changing that would alter every positive number the /// product shows. So the two functions are inverse across the decimal @@ -63,7 +55,7 @@ /// not across a positive sign -- entry accepts a spelling display never /// produces. /// -/// @par The digits are locale data too (morph#591) +/// @par The digits are locale data too /// `NumericLocale::zeroDigit` is the locale's DIGIT ZERO, and the ten digits /// are the ten code points contiguous from it. One base is sufficient rather /// than a ten-element table because a Unicode decimal digit set *is* ten @@ -77,9 +69,9 @@ /// /// @par Entry accepts more digit spellings than display emits /// `normalizeLocaleNumber` accepts a digit in `[zeroDigit, zeroDigit + 9]` *or* -/// in `['0', '9']`; `formatCanonicalNumber` emits only the former. That -/// asymmetry is the rule morph#596 already set for signs, applied to digits: an -/// ASCII `'+'` is accepted in every locale because the locale's own spelling is +/// in `['0', '9']`; `formatCanonicalNumber` emits only the former. That is the +/// same asymmetry the signs have, applied to digits: an ASCII `'+'` is +/// accepted in every locale because the locale's own spelling is /// on no keyboard, and an ASCII `'5'` is accepted in an `ar_EG` locale for /// exactly the same reason. A user with an ASCII keyboard in a native-digit /// locale would otherwise be unable to enter a number at all. As with the @@ -101,7 +93,7 @@ namespace morph::render { /// Every member is defaulted to its `"C"`-locale spelling, so a /// default-constructed `NumericLocale` is the identity transform in both /// directions and a caller naming only the members it cares about gets the -/// previous five-parameter defaults exactly. +/// `"C"` spelling for the rest. struct NumericLocale { /// The locale's decimal-point string, e.g. `","`. Empty means the locale /// has no decimal separator (an integer-only entry). @@ -140,9 +132,8 @@ struct CodePoint { /// rather than some salvaged value. That strictness is load-bearing rather than /// pedantic. The digit test below is a *range* test on the decoded value, so a /// lenient decoder that let the overlong `C0 B5` through would read it as -/// U+0035 and accept it as the digit `'5'` -- in the default ASCII locale, -/// where the previous byte-range scan rejected that input. Rejecting it here is -/// what keeps the default behaviour byte-identical. +/// U+0035 and accept it as the digit `'5'` -- including in the default ASCII +/// locale, where that input is not a digit at all. /// @param text The remainder of the entry, starting at the scan position. /// @return The decoded code point, or a `length` of `0` when @p text does not /// start with a well-formed sequence. @@ -223,12 +214,10 @@ inline void appendUtf8(std::string& out, char32_t value) { /// digit rewritten into the locale's set. /// /// Anything that is not an ASCII digit is copied through verbatim. That arm is -/// not dead code for a caller that honours the contract, but it is what keeps -/// `formatCanonicalNumber` byte-identical for one that does not: the function -/// has always passed non-digits out unchanged, so a `"12.34.56"` handed to the -/// display edge kept its second `'.'` rather than becoming some code point -/// below the digit base. morph#591 widened what a digit *is*, not what the -/// function does with text that has none. +/// not dead code for a caller that honours the contract, but it is what bounds +/// the damage for one that does not: a `"12.34.56"` handed to the display edge +/// keeps its second `'.'` rather than becoming some code point below the digit +/// base. Only digits are rewritten; text that has none passes through. /// @param out The display string to append to. /// @param base The locale's DIGIT ZERO code point (see `digitBase`). /// @param chr One byte of canonical text. @@ -327,12 +316,12 @@ struct DigitMatch { /// separate statements about the entry, and reading them as one made neither /// clear. /// -/// It counts *digits*, not bytes, in the locale's set as well as in ASCII -/// (morph#591). Scanning byte by byte was correct only while a digit was one -/// byte: a two-byte U+0665 would have reset the run-length counter on its own -/// continuation byte, so `"\u0661\u066c\u0660\u0665\u0660"` -- what -/// `formatCanonicalNumber` emits for `1050` in an `ar_EG` locale -- would be -/// rejected as badly grouped and the pair would not round-trip. +/// It counts *digits*, not bytes, in the locale's set as well as in ASCII. A +/// byte-by-byte scan is only correct while a digit is one byte: a two-byte +/// U+0665 would reset the run-length counter on its own continuation byte, so +/// `"\u0661\u066c\u0660\u0665\u0660"` -- what `formatCanonicalNumber` emits +/// for `1050` in an `ar_EG` locale -- would be rejected as badly grouped and the +/// pair would not round-trip. /// /// Characters this function does not recognise are simply not digits — the /// normalising scan is what rejects them, and it rejects them whatever this @@ -388,9 +377,9 @@ struct DigitMatch { /// The empty case is why this is a function and not an inline `starts_with`: /// `rest.starts_with("")` is `true` at every index, so an empty separator /// matched inline would swallow the whole entry one zero-length step at a time. -/// Every call site used to spell the guard as `!sep.empty() && ...`, and two of -/// those conjunctions were what put `normalizeLocaleNumber` over clang-tidy's -/// cognitive-complexity threshold once the digit scan arrived. +/// Spelling the guard as `!sep.empty() && ...` at each call site instead adds +/// two conjunctions to `normalizeLocaleNumber`, which is enough to put it over +/// clang-tidy's cognitive-complexity threshold. /// @param rest The remainder of the entry, starting at the scan position. /// @param separator The locale's spelling of this separator; empty means the /// locale has none, and matches nothing. @@ -404,8 +393,8 @@ struct DigitMatch { /// /// Two spellings count. @p localeSign is the locale's own, matched as a whole /// string so that U+2212 and the bidi-control-prefixed forms -- two and three -/// code points -- match at all; a single `char` could express none of them -/// (morph#583, morph#596). @p asciiSign counts as well, in every locale: U+2212 +/// code points -- match at all; a single `char` can express none of them. +/// @p asciiSign counts as well, in every locale: U+2212 /// and the bidi marks are on no keyboard, so matching only the locale's /// spelling would reject the sign the user can actually type. Neither `'-'` nor /// `'+'` has a second reading in a numeric entry, so this is not the kind of @@ -435,7 +424,7 @@ struct SignMatch { /// The number of bytes the sign occupies; `0` when there is no sign there. std::size_t length = 0; /// What the canonical text gains: `"-"` for a negative, empty for a - /// positive, which is accepted and dropped (morph#596). + /// positive, which is accepted and dropped. std::string_view emits; }; @@ -444,10 +433,10 @@ struct SignMatch { /// Both signs in one function, and the emitted text carried back with the /// length, so the normalising scan below has a *single* sign branch with no /// inner "which sign was it" test. That is not only tidier: two branches with -/// two inner tests each took `normalizeLocaleNumber` from a cognitive -/// complexity of 23 to 28, over clang-tidy's threshold of 25. The asymmetry -/// between the two signs lives here, in the one place that decides it, rather -/// than in the scan. +/// two inner tests each put `normalizeLocaleNumber` at a cognitive complexity +/// of 28, over clang-tidy's threshold of 25; with the single branch it sits at +/// 23. The asymmetry between the two signs lives here, in the one place that +/// decides it, rather than in the scan. /// /// The negative sign is tried first. The order is not load-bearing for any /// locale Qt 6.11.2 reports -- `starts_with` is an exact prefix match and no @@ -483,17 +472,17 @@ struct SignMatch { /// yields `std::nullopt` rather than a best-effort guess. The decimal point /// counts as output, so a sign placed straight after the separator ("`,-5`" in /// a de-DE locale) is rejected -- matching the QML mirror in -/// `src/qt/forms/qml/DynamicForm.qml`, which has always rejected it (morph#497). +/// `src/qt/forms/qml/DynamicForm.qml`, which rejects it too. /// -/// @par Grouping is validated, not stripped (morph#574) +/// @par Grouping is validated, not stripped /// A group separator is only dropped where a group separator can legally be: /// preceded by one to three digits, followed by exactly three more, and never /// after the decimal separator. Anything else is malformed and reported as -/// such. Stripping unconditionally instead is a wrong *value*, not a rejected -/// one: a de-DE user typing the US form `"1.5"` into a price field submitted -/// `15`, and nothing downstream could tell -- the result is a perfectly valid -/// number, ten times too large. `"1.50"` gave `150`, `"1.2.3.4"` gave `1234`, -/// and the en-US mirror image `"1,5"` gave `15`. +/// such. Stripping unconditionally instead yields a wrong *value*, not a +/// rejected one, and nothing downstream can tell: a de-DE user typing the US +/// form `"1.5"` into a price field would submit `15` -- a perfectly valid +/// number, ten times too large. `"1.50"` would give `150`, `"1.2.3.4"` would +/// give `1234`, and the en-US mirror image `"1,5"` would give `15`. /// /// @par The two separators must differ /// When `groupSeparator` is non-empty and equal to `decimalSeparator` the entry @@ -507,19 +496,19 @@ struct SignMatch { /// The result is `.`-decimal and digit-only, but is **not** narrowed to /// `-?[0-9]+(\.[0-9]+)?`: a bare "`.`", a leading "`.5`" and a trailing "`5.`" /// are passed through, exactly as that same QML mirror passes them. Tightening -/// one side alone would put the two control edges back out of step, so the shape -/// is documented here rather than changed. +/// one side alone would put the two control edges out of step, so the shape is +/// documented here rather than narrowed. /// /// Separators are matched as whole strings, so a multi-byte one (e.g. U+202F) /// works; matching them before the digit scan is what keeps their continuation /// bytes from being mistaken for stray non-digit characters. /// -/// @par The negative sign is matched as a whole string too (morph#583) +/// @par The negative sign is matched as a whole string too /// `negativeSign` is matched the same way, which is what lets a locale whose /// sign is U+2212, or is prefixed by a bidi control mark, be entered at all -- -/// 77 of the 711 locales Qt 6.11.2 knows. Before this the sign was the literal -/// byte `'-'`, so `formatCanonicalNumber` emitted a sign this function then -/// rejected, and the pair was not inverse for those locales. +/// 77 of the 711 locales Qt 6.11.2 knows. Matched as a literal byte `'-'` +/// instead, `formatCanonicalNumber` would emit a sign this function rejected +/// and the pair would not be inverse for those locales. /// /// @par ASCII `'-'` stays accepted whatever the locale /// A bare `'-'` is accepted in the leading position in addition to @@ -527,8 +516,8 @@ struct SignMatch { /// only the locale's own spelling would reject the sign the user can actually /// type and leave them no way to enter a negative number at all. The hyphen has /// no second reading in a numeric entry, so accepting it is not the kind of -/// guess morph#574 forbids -- that was about producing a wrong *value*, and -/// this produces the only value the input can mean. +/// guess the grouping rule forbids -- that one is about producing a wrong +/// *value*, and this produces the only value the input can mean. /// /// @par An empty `negativeSign` means the ASCII default, not "no sign" /// Unlike a group separator, there is no locale without a negative sign, so an @@ -538,7 +527,7 @@ struct SignMatch { /// nothing would turn `-5` into `5` silently -- a wrong value, not a rejected /// one. /// -/// @par A leading positive sign is accepted and dropped (morph#596) +/// @par A leading positive sign is accepted and dropped /// `positiveSign` is matched exactly like `negativeSign` -- the locale's own /// spelling as a whole string, plus a bare ASCII `'+'` in every locale. Of the /// 711 locales Qt 6.11.2 knows, 54 spell it as more than one code point @@ -546,9 +535,9 @@ struct SignMatch { /// `ckb_IQ`); the other 657 use the bare `'+'`. Unlike the negative side there /// is no U+2212 analogue, so *every* non-ASCII spelling here is multi-code-point /// and whole-string matching is the only thing that can match any of them. -/// Before this, a leading `'+'` fell through to the "any other character is -/// malformed" arm and an explicitly-positive entry was rejected in every -/// locale, `"C"` included. +/// Without this branch a leading `'+'` falls through to the "any other +/// character is malformed" arm, and an explicitly-positive entry is rejected in +/// every locale, `"C"` included. /// /// @par The sign is **dropped**, and `formatCanonicalNumber` never emits one /// This is a deliberate asymmetry with the negative sign, not an oversight. @@ -558,9 +547,8 @@ struct SignMatch { /// form looks like -- `5` would become `+5` on screen. So the two functions are /// *not* strict inverses across a positive sign: entry accepts a spelling /// display never produces. That is the only shape that adds acceptance without -/// changing a single rendered value, and it is why morph#596 is an enhancement -/// rather than the repaired round trip morph#583 was. Written down in -/// `docs/spec/forms/forms.md` as well, under "Locale data formatting". +/// changing a single rendered value. Written down in `docs/spec/forms/forms.md` +/// as well, under "Locale data formatting". /// /// @par An empty `positiveSign` leaves the ASCII `'+'` /// Here empty really can mean "match nothing extra", because there is no @@ -568,14 +556,14 @@ struct SignMatch { /// reject an entry, never produce a value of the wrong sign. The bare ASCII /// `'+'` stays accepted regardless. /// -/// @par The locale's own digits are accepted, and so are ASCII ones (morph#591) +/// @par The locale's own digits are accepted, and so are ASCII ones /// A digit is accepted when its code point is in /// `[zeroDigit, zeroDigit + 9]` -- a Unicode decimal digit set is ten /// contiguous code points by definition (UAX #44) -- *or* in `['0', '9']`. 76 -/// of the 711 locales Qt 6.11.2 knows use a non-ASCII `zeroDigit`; before this -/// their users could not enter a number at all, because the scan compared a -/// single byte against the ASCII range and the very first byte of U+0665 -/// failed it. The second acceptance is the same rule as the ASCII `'-'` and +/// of the 711 locales Qt 6.11.2 knows use a non-ASCII `zeroDigit`; without the +/// first acceptance their users could not enter a number at all, because a scan +/// comparing a single byte against the ASCII range fails on the very first byte +/// of U+0665. The second acceptance is the same rule as the ASCII `'-'` and /// `'+'` above, for the same reason: a user with an ASCII keyboard in an /// `ar_EG` locale has to be able to type `5`. It costs nothing, because the /// canonical output spells every digit in ASCII whatever the input spelled it, @@ -585,11 +573,10 @@ struct SignMatch { /// `"\u06655"` -- one Arabic-Indic digit and one ASCII digit -- is malformed, /// not `"55"`. The two families are each accepted whole; interleaving them is /// not a spelling any keyboard or any display edge produces, and rejecting it -/// matches the existing strictness about a sign anywhere but the leading -/// position. It is a choice rather than a consequence, so it is stated here and -/// pinned by a test on both edges. Note that when `zeroDigit` is the ASCII -/// `"0"` the two families are the same set, so nothing can mix and the rule is -/// invisible -- which is why it costs no existing caller anything. +/// matches the strictness about a sign anywhere but the leading position. It is +/// a choice rather than a consequence, so it is stated here and pinned by a +/// test on both edges. When `zeroDigit` is the ASCII `"0"` the two families are +/// the same set, so nothing can mix and the rule is invisible. /// /// @param text The locale-formatted entry, e.g. `"1.050,25"`. /// @param loc The locale facts. Designated initialisers are the intended @@ -630,7 +617,6 @@ struct SignMatch { // guard in the sign branch below still believes nothing has been // emitted, and a sign placed straight after the separator // ("`,-5`" in a de-DE locale) is accepted as if it were leading. - // morph#497. sawAnyOutput = true; i += point; continue; @@ -644,10 +630,10 @@ struct SignMatch { if (sawAnyOutput) { return std::nullopt; // sign injection past the leading position } - // Empty for a positive sign, which is dropped rather than carried - // (morph#596). `sawAnyOutput` is set either way, so "+-5", "++5" - // and "1+2" stay malformed: consuming a sign counts as output even - // when it contributes no character. + // Empty for a positive sign, which is dropped rather than + // carried. `sawAnyOutput` is set either way, so "+-5", "++5" and + // "1+2" stay malformed: consuming a sign counts as output even when + // it contributes no character. canonical += sign.emits; sawAnyOutput = true; i += sign.length; @@ -687,34 +673,21 @@ struct SignMatch { /// it (see "Grouping is validated, not stripped" on that function). A /// default-constructed `NumericLocale` is the identity transform. /// -/// @par What this paragraph used to say, and why it was wrong (morph#597) -/// It claimed grouping was "never accepted back on entry" and that -/// `normalizeLocaleNumber` "strips it unconditionally" -- the pre-morph#574 -/// behaviour, and false in two opposite directions at once. Measured on -/// `be64026a`: `normalizeLocaleNumber("1.050,25", ",", ".")` is `"1050.25"`, -/// so grouping *is* accepted back; `normalizeLocaleNumber("1.5", ",", ".")` is -/// `std::nullopt`, so it is *not* stripped unconditionally -- unconditional -/// stripping is exactly what would have made that entry `15`, the silent -/// ten-times-wrong value morph#574 exists to prevent. The spec -/// (`docs/spec/forms/forms.md`, "Grouping is validated, never merely -/// stripped") and the code already agreed; only this comment was stale. -/// /// The sign is emitted as `NumericLocale::negativeSign`, matching what -/// `normalizeLocaleNumber` accepts back (morph#583); an empty view is read as +/// `normalizeLocaleNumber` accepts back; an empty view is read as /// `"-"` rather than as "no sign", because formatting a negative to no sign at /// all is a silently wrong value. /// -/// @par The digits are emitted in the locale's set (morph#591) +/// @par The digits are emitted in the locale's set /// Each canonical `'0'`-`'9'` is emitted as the code point that far above /// `NumericLocale::zeroDigit`, so an `ar_EG` caller sees `"\u0665"` where the -/// canonical text said `'5'`. This edge *had* to move with the entry edge: it -/// used to copy the canonical ASCII bytes out unchanged, so teaching entry to -/// accept U+0665 while display kept emitting `'5'` would have left the pair no -/// longer inverse, which is the round trip `docs/spec/forms/forms.md` requires. -/// With the default `zeroDigit` of `"0"` the offset is zero and every byte is -/// the one this function emitted before. -/// -/// @par There is no positive-sign emission, deliberately (morph#596) +/// canonical text said `'5'`. This edge has to match the entry edge: entry +/// accepts U+0665, so display emitting a plain `'5'` would leave the pair not +/// inverse, and `docs/spec/forms/forms.md` requires that round trip. With the +/// default `zeroDigit` of `"0"` the offset is zero and the canonical ASCII +/// bytes are copied out unchanged. +/// +/// @par There is no positive-sign emission, deliberately /// A positive number is displayed with no sign at all, in every locale, and /// this function ignores `NumericLocale::positiveSign` entirely. /// `normalizeLocaleNumber` *accepts* a leading positive sign and drops it, so @@ -722,9 +695,9 @@ struct SignMatch { /// never produces. Emitting it is what would be the defect -- /// `QLocale::positiveSign()` is `'+'` in 657 of the 711 locales Qt 6.11.2 /// knows, so emitting it would turn every positive number in every form from -/// `5` into `+5`, a visible product change with no reported need behind it. -/// Rejecting text the display edge produced is the morph#583 shape and is not -/// what happens here; producing text no display edge asked for would be. +/// `5` into `+5`, a visible product change with no need behind it. The failure +/// this pair guards against is one edge producing text the other rejects; +/// emitting a sign nothing asks for would create exactly that. /// @param canonicalText Canonical `-?[0-9]+(\.[0-9]+)?` text. /// @param loc The locale facts; `positiveSign` is not read. /// @return The locale-formatted display text. diff --git a/include/morph/session/session.hpp b/include/morph/session/session.hpp index 016f6cc1..93cfe7ef 100644 --- a/include/morph/session/session.hpp +++ b/include/morph/session/session.hpp @@ -111,7 +111,7 @@ struct Principal { /// **An implementation that switches or matches on `actionType` must handle the /// empty case explicitly**, or it will hit its default arm on exactly those two /// disclosure verbs. Whether that fails open or closed is the implementation's -/// choice, but it has to be a choice (morph#500). +/// choice, but it has to be a choice. /// /// Default implementation supplied by the framework is `AllowAllAuthorizer`. Real /// deployments install a custom subclass that checks principal claims, action @@ -268,7 +268,7 @@ class ScopedContext { /// @brief Installs @p ctx as the thread-local context until the scope exits. /// @param ctx Context whose address is stored; must outlive this object. explicit ScopedContext(const Context& ctx) : _prev{tlsCurrent()} { tlsCurrent() = &ctx; } - /// @brief Restores the previously active thread-local context. + /// @brief Restores the thread-local context that was active at construction. ~ScopedContext() { tlsCurrent() = _prev; } ScopedContext(const ScopedContext&) = delete; ScopedContext& operator=(const ScopedContext&) = delete; diff --git a/include/morph/session/session_auth.hpp b/include/morph/session/session_auth.hpp index c6c1cda5..80b70554 100644 --- a/include/morph/session/session_auth.hpp +++ b/include/morph/session/session_auth.hpp @@ -523,7 +523,7 @@ class SigningAuthorizer : public IAuthorizer { /// Receives the verified token plus the target ids; return `false` to deny. /// The action id is **empty** for the `instances` and `schemas` envelopes -- /// see `IAuthorizer::authorize`'s own doc comment for why, and handle that - /// case explicitly rather than letting it reach a default arm (morph#500). + /// case explicitly rather than letting it reach a default arm. /// The default (empty) admits any validly-signed, unexpired token. using Policy = std::function; From 2c82aaf10c63e3ab82a21a197aad4bf243862847 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 23 Sep 2026 18:23:11 +0200 Subject: [PATCH 3/5] comments(core): the constraint without the changelog `core` carried the densest history in the tree: 77 ticket citations in `bridge.hpp` alone, a commit hash in `backend.hpp`'s allocation note, and several blocks written as the story of a refactor -- a before/after allocation table, "what had to be preserved, and where it now lives", "what morph#593 established", "the four `*Async` twins morph#571 removed". What those blocks were protecting is the reasoning, and it stays: the four invariants `BridgeSink`'s settle path holds, why `notify`-style liveness checks are two steps and what closes the window, why `_attachMtx` is not held across a dispatch, why the gate advances over a contiguous run of released tickets rather than jumping, why an out-of-order release is recorded rather than applied. Those are current facts about current constraints and several of them are still twenty lines long. What went is the narrative around them, and the tracker round trip. The three clang-tidy suppression stamps keep their verification status -- what was re-checked, at which pinned version, and what would change the verdict -- without naming the tickets that deleted the scripts that used to check. Assert message strings in `registry.hpp` and `bridge.hpp` also named a ticket; those are text a user reads, so they now point at `docs/spec/core/registry.md` instead. No other code changed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- include/morph/core/async.hpp | 2 +- include/morph/core/backend.hpp | 153 ++++--- include/morph/core/bridge.hpp | 406 ++++++++---------- include/morph/core/callback_scope.hpp | 16 +- include/morph/core/completion.hpp | 27 +- .../morph/core/detail/execute_order_gate.hpp | 69 ++- .../morph/core/detail/instance_directory.hpp | 22 +- include/morph/core/detail/reply_router.hpp | 2 +- include/morph/core/executor.hpp | 1 - include/morph/core/file_io_ops.hpp | 22 +- include/morph/core/registry.hpp | 83 ++-- include/morph/core/remote.hpp | 65 ++- include/morph/core/strand.hpp | 10 +- include/morph/core/wire.hpp | 22 +- 14 files changed, 415 insertions(+), 485 deletions(-) diff --git a/include/morph/core/async.hpp b/include/morph/core/async.hpp index dc4f6357..a6d9ff39 100644 --- a/include/morph/core/async.hpp +++ b/include/morph/core/async.hpp @@ -23,7 +23,7 @@ /// A consumer that wants the primitives and reaches for `bridge.hpp` — the /// obvious header, and the one every example includes — pays roughly three /// times over for a schema generator and a JSON codec it never calls. This -/// header exists so the cheap path has a name (morph#573, step 4). +/// header exists so the cheap path has a name. /// /// It is a facade and nothing else: it declares no symbol of its own, so /// including it is exactly equivalent to including the four headers below. diff --git a/include/morph/core/backend.hpp b/include/morph/core/backend.hpp index c321ee9e..95b39fc5 100644 --- a/include/morph/core/backend.hpp +++ b/include/morph/core/backend.hpp @@ -49,9 +49,9 @@ namespace detail { /// callables cost up to three allocations per dispatch whichever path the call /// takes. Stateless operations parameterised on the action cost none: the /// per-`(Model, Action)` behaviour is a compile-time constant, addressed rather -/// than copied. Measured on `master` @ `a9cb5649` with `morph_bench_alloc`, -/// this shape and the two `string_view` ids below together removed 3 of 17 -/// allocations per local round trip (morph#572, Part A). +/// than copied. Measured with `morph_bench_alloc`: this shape and the two +/// `string_view` ids below together account for 3 of the allocations a local +/// round trip would otherwise make, out of 17. /// /// @par Lifetime contract for the callables /// `serializeAction` and `localOp` take the action as an opaque pointer and do @@ -200,17 +200,17 @@ struct PromoteRequest { /// `Completion` may block its own thread until that `Completion` /// settles. /// -/// This is the one thing `bindModel`'s signature cannot say, and morph#593 is -/// what happens when it is not said: two shipped backends both return an -/// unsettled `Completion` from `bindModel`, and `Bridge::registerHandlerImpl` -/// — a synchronous entry point that hands its caller a `BridgeHandler` usable -/// on the next line — must wait for one of them and must not wait for the -/// other. From the `Completion` alone the two are indistinguishable. +/// This is the one thing `bindModel`'s signature cannot say, and it has to be +/// said: two shipped backends both return an unsettled `Completion` from +/// `bindModel`, and `Bridge::registerHandlerImpl` — a synchronous entry point +/// that hands its caller a `BridgeHandler` usable on the next line — must wait +/// for one of them and must not wait for the other. From the `Completion` +/// alone the two are indistinguishable. /// -/// It is deliberately **not** the `bool` the surface removed (see "The -/// structural registration surface" below, point 1). That `bool` chose -/// *which verb to call*, so every call site carried two paths and a backend -/// could be half-migrated. This one chooses nothing: there is still exactly +/// It is deliberately **not** a `bool` selecting a verb (see "The structural +/// registration surface" below, point 1). Such a `bool` would make every call +/// site carry two paths and let a backend be half-migrated. This chooses +/// nothing: there is still exactly /// one verb, called unconditionally, and exactly one continuation. It says /// only whether the thread that issued the call is allowed to stop and wait /// for the continuation it already registered. @@ -237,7 +237,7 @@ enum class BindWait : std::uint8_t { /// - `QtWebSocketBackend` with `Config::asyncRegistrationEnabled` set: the /// reply arrives through the Qt event loop of the thread that issued the /// call, so waiting is a deadlock. On a WASM main thread it aborts the - /// page (morph#568), which is the case that surface exists for. + /// page, which is the case this surface exists for. /// - `SynchronousBackendAdapter`: it exists precisely to move a blocking /// call off the caller's thread, so a caller that then waits for it has /// bought nothing — and if the caller happens to be running on the @@ -375,36 +375,32 @@ struct IBackend { // ── The structural registration surface ────────────────────────────── // - // `bindModel`/`promoteModel` are what the five verbs above become once the - // continuation stops being optional. Two differences carry the whole - // change, and both are visible in the signature rather than in a comment: + // `bindModel`/`promoteModel` are the five verbs above with the continuation + // made mandatory. Two properties carry the design, and both are visible in + // the signature rather than only in a comment: // // 1. **The continuation is not opt-in.** There is no `bool` saying "I // have no async path, call the other one" — so no call site carries a // second path, and a backend cannot be *half* migrated. A blocking - // backend satisfies the surface unchanged through the default - // implementations below, or without blocking the caller at all - // through `SynchronousBackendAdapter`. + // backend satisfies the surface through the default implementations + // below, or without blocking the caller at all through + // `SynchronousBackendAdapter`. // - // What morph#593 established is that removing *that* bool also - // removed something else the call site needed and that is not the - // same question: whether the thread that called `bindModel` is - // allowed to wait for the continuation it just registered. Two + // Whether the *calling thread* may wait for that continuation is a + // separate question, and the signature cannot answer it either: two // shipped backends return an unsettled `Completion` and give opposite // answers (`SocketBackend`: yes, its I/O thread settles it; // `QtWebSocketBackend` under `asyncRegistrationEnabled`: no, waiting // deadlocks the event loop the reply arrives on). `bindWaitPolicy()` - // below restores exactly that one bit and nothing else — it never - // selects a verb, so the "second path" the removed bool created does - // not come back with it. + // below carries exactly that one bit and nothing else — it never + // selects a verb, so it creates no second path. // // 2. **The delivery thread is a parameter.** `Completion` posts its // handlers to the executor it was built with, so the continuation runs // where @p cbExec says and nowhere else — the backend does not choose. - // That is the whole of the threading contract the four `*Async` - // twins morph#571 removed could only state in prose: they asked - // every backend author to deliver on a thread from which `~Bridge` - // could not run concurrently, and nothing could check it. See + // A threading contract stated only in prose would ask every backend + // author to deliver on a thread from which `~Bridge` cannot run + // concurrently, and nothing could check it. See // docs/spec/core/backend.md, "The threading contract, and the half // the surface does not close". // It does not by itself make a `~Bridge` race impossible: it moves the @@ -414,13 +410,11 @@ struct IBackend { // nowhere" cannot be expressed — a null executor would silently drop // every continuation (see `Completion`'s constructor). // - // morph#571 retired the four optional `*Async` twins. The synchronous - // verbs above remain, but no longer as a surface any caller chooses: - // they are what `bindModelBlocking` — and therefore the *default* - // `bindModel` — dispatches to, one request shape at a time. Nothing in - // the tree calls them directly any more, which is why a backend that - // overrides only `registerModel` still works through `bindModel` - // unchanged. + // The synchronous verbs above remain, but not as a surface any caller + // chooses: they are what `bindModelBlocking` — and therefore the *default* + // `bindModel` — dispatches to, one request shape at a time. Nothing in the + // tree calls them directly, which is why a backend that overrides only + // `registerModel` still works through `bindModel`. /// @brief Acquires a model instance: the structural counterpart of /// `registerModelWithContext` / `registerModelShared` / `attachModel`. @@ -432,7 +426,7 @@ struct IBackend { /// anything keeps its current behaviour bit for bit, including its current /// blocking behaviour: the default **blocks the calling thread** for as /// long as the underlying synchronous verb does. A backend with a genuine - /// non-blocking path (morph#568's `QtWebSocketBackend`) overrides this and + /// non-blocking path (`QtWebSocketBackend`) overrides this and /// settles the `Completion` when its reply arrives; a blocking backend that /// must not block its caller is wrapped in `SynchronousBackendAdapter`, /// which moves the blocking call to an executor it names. @@ -502,8 +496,8 @@ struct IBackend { /// `Bridge::switchBackend`'s staging phase, and /// `Bridge::installReconnectHandler`'s handler, which is the one that runs /// on the backend's own transport thread and so is the one a wrong answer - /// deadlocks outright (morph#615). The asynchronous entry points never wait - /// and never consult it. + /// deadlocks outright. The asynchronous entry points never wait and never + /// consult it. /// /// A backend that returns `kCallerMayBlock` (the default) commits to /// settling every `Completion` it returns exactly once without any further @@ -574,11 +568,10 @@ struct IBackend { /// The same dispatch as `execute`, with the result delivered to a sink the /// caller already owns instead of to a fresh `Completion` the caller then /// has to forward into its own. `Bridge::executeVia` hands down a sink that - /// **is** the typed completion state the caller was given, which is what - /// removes the six-allocation forwarding block between the two (morph#572, - /// Part B): the erased `CompletionState`, the `.then` and `.onError` - /// closures, their two handler vectors, and one of the two posted settle - /// tasks. + /// **is** the typed completion state the caller was given, so there is no + /// forwarding block between the two — no erased `CompletionState`, no + /// `.then`/`.onError` closures, no second pair of handler vectors, and one + /// posted settle task instead of two. /// /// @par Why this has a default rather than being pure /// `IBackend::execute` is implemented by five production backends and @@ -596,7 +589,7 @@ struct IBackend { /// answer `cancelPending` must track the sink, not a state of its own. /// Throwing out of this call is permitted and means the dispatch never /// started — `Bridge::executeVia` undoes its pending count and its deadline - /// on that path, exactly as it does for `execute` (morph#502). + /// on that path, exactly as it does for `execute`. /// /// @param mid Target model id. /// @param call Bundled action; moved from. @@ -767,10 +760,9 @@ struct ClientTimeoutError : std::runtime_error { /// every verb to it, overriding only `bindModel`/`promoteModel` to run the /// wrapped backend's *synchronous* control call on an executor this adapter /// names, then settle the returned `Completion`. That is the whole trick, and -/// it is why migrating the rest of morph#522's set is a set of migrations -/// rather than a set of rewrites: a backend with no non-blocking path of its -/// own (every backend in the tree except `QtWebSocketBackend`) reaches the new -/// surface by being wrapped, not by being rewritten. +/// it is why a backend with no non-blocking path of its own (every backend in +/// the tree except `QtWebSocketBackend`) reaches the structural surface by +/// being wrapped rather than by being rewritten. /// /// @par What it does and does not change /// The wrapped backend still blocks — nothing here makes a nested event loop @@ -779,7 +771,7 @@ struct ClientTimeoutError : std::runtime_error { /// caller's thread returns from `bindModel` immediately with an unresolved /// `Completion`. A single-threaded WASM main thread has no such executor to /// offer and is therefore not what this adapter is for; that case needs a -/// backend with a genuinely non-blocking path (morph#568). +/// backend with a genuinely non-blocking path. /// /// @par Why the executor is required rather than optional /// "Where does the blocking happen" is the only question this class exists to @@ -802,7 +794,7 @@ struct ClientTimeoutError : std::runtime_error { /// the wrapped backend's transport thread, so a backend whose reply can only /// be delivered by the thread that is running the reconnect handler does not /// wait on itself. Whether that is enough to settle `SocketBackend`'s -/// documented reconnect hazard is morph#569's question, not a claim made here. +/// documented reconnect hazard is not a claim made here. // NOLINTNEXTLINE(cppcoreguidelines-special-member-functions) class SynchronousBackendAdapter : public detail::IBackend { public: @@ -885,9 +877,9 @@ class SynchronousBackendAdapter : public detail::IBackend { // ── Everything else is forwarded unchanged ─────────────────────────── // - // A decorator has to forward every verb it does not reshape. Since - // morph#571 those are the synchronous verbs only: a wrapped backend has - // no non-blocking path of its own left to forward, because the one verb + // A decorator has to forward every verb it does not reshape. Those are the + // synchronous verbs only: a wrapped backend has no non-blocking path of + // its own left to forward, because the one verb // that could carry one — `bindModel` — is the verb this adapter // reshapes, and a backend that already has a non-blocking `bindModel` // has no reason to be wrapped. @@ -976,8 +968,8 @@ class SynchronousBackendAdapter : public detail::IBackend { /// same reason `bindWaitPolicy()` is not: the two verbs this adapter /// reshapes produce completions the wrapped backend has never heard of. /// A `bindModel` here settles from a task on `_control`, so - /// `_inner->cancelPending` reaches nothing of it — before morph#619 a bind - /// dispatched through this adapter went on to resolve **successfully** + /// `_inner->cancelPending` reaches nothing of it: without this override a + /// bind dispatched through this adapter goes on to resolve **successfully** /// after cancellation, which is the exact opposite of what /// `IBackend::cancelPending` promises ("after this call, any later /// `setValue`/`setException` on those states is a no-op"). @@ -995,10 +987,10 @@ class SynchronousBackendAdapter : public detail::IBackend { /// cancellation flag next to its promise, and this verb **sets that flag /// before rejecting**: a task still queued on `_control` sees it when it /// reaches the head of the strand and returns without calling `op()`, so - /// the blocking control call never reaches the wrapped backend at all - /// (morph#636). Without it the caller was told the bind was cancelled while - /// the registration went through anyway — a live instance on a backend - /// whose `Bridge` is gone, which nothing will ever `deregisterModel`. + /// the blocking control call never reaches the wrapped backend at all. + /// Without the flag the caller is told the bind was cancelled while the + /// registration goes through anyway — a live instance on a backend whose + /// `Bridge` is gone, which nothing will ever `deregisterModel`. /// /// **What this still does not do:** a task already *inside* `op()` cannot /// be recalled. Only the queued-but-not-started window is closed, which is @@ -1019,8 +1011,8 @@ class SynchronousBackendAdapter : public detail::IBackend { if (auto pending = pendingWeak.lock()) { // Flag first, promise second. A task that reads the flag after // this store declines to run; one that read it just before - // finds its promise already rejected by the line below, which - // is the pre-morph#636 outcome and the narrowest window left. + // finds its promise already rejected by the line below. That + // is the narrowest window this ordering leaves. pending->cancelled.store(true, std::memory_order_release); pending->promise.reject(exc); } @@ -1047,10 +1039,10 @@ class SynchronousBackendAdapter : public detail::IBackend { private: /// @brief One dispatched control call: its promise and its cancellation flag. /// - /// The two travel together because `cancelPending` has to act on both, and - /// acting on only the promise is the defect morph#636 recorded — the queued - /// task went on to make the blocking control call the caller had just been - /// told was cancelled. The strand task holds the only `shared_ptr` to this + /// The two travel together because `cancelPending` has to act on both: + /// acting on the promise alone leaves the queued task free to make the + /// blocking control call the caller has just been told was cancelled. + /// The strand task holds the only `shared_ptr` to this /// record; `_pending` holds `weak_ptr`s, so an entry expires by itself when /// the task is destroyed. struct PendingControl { @@ -1092,10 +1084,10 @@ class SynchronousBackendAdapter : public detail::IBackend { // between would otherwise find an empty list and leave a completion // that is genuinely pending uncancelled. Rejecting a promise whose task // has not started yet is safe — the task's own `resolve` then finds the - // state ready and returns (morph#619). + // state ready and returns. trackPending(pending); _control.post(kControlStrand, [pending, op = std::move(op)]() mutable { - // Checked *before* `op()`, which is the whole of morph#636: a + // Checked *before* `op()`: a // promise settled by `cancelPending` makes the reply a no-op but // says nothing about the call, and this task is the last place that // can decline to make it. Read with acquire against @@ -1120,8 +1112,8 @@ class SynchronousBackendAdapter : public detail::IBackend { /// here expires exactly when that task is destroyed — "still pending" needs /// no separate bookkeeping and no erase on the success path. /// - /// Swept on the same amortised schedule as `LocalBackend::trackPending` - /// (morph#528): dead entries are reclaimed only when the list reaches + /// Swept on the same amortised schedule as `LocalBackend::trackPending`: + /// dead entries are reclaimed only when the list reaches /// `_compactAt`, which each sweep re-arms at twice the surviving count, so /// the per-dispatch cost is O(1) and the list stays bounded at twice the /// live count plus the floor. Control calls are serialised onto one strand, @@ -1318,7 +1310,7 @@ class LocalBackend : public detail::IBackend { /// one definition of what a local dispatch does whichever entry point a /// caller uses. The extra `CompletionState` and its adapter sink are the /// price of the `Completion`-returning shape — which is exactly the cost - /// `executeInto` exists to let `Bridge` stop paying (morph#572, Part B). + /// `executeInto` exists to let `Bridge` avoid. /// /// @par Why this is `final` /// `Bridge::executeVia` calls `executeInto`, not `execute`. A subclass that @@ -1540,9 +1532,9 @@ class LocalBackend : public detail::IBackend { /// correspondingly bounded at twice the live count (plus the floor), which /// is the whole price of dropping the per-dispatch scan. /// - /// Before morph#528 this swept on *every* append, so admitting one call with - /// `n` in flight cost `n` atomic `weak_ptr::expired()` loads under - /// `_pendingMtx` and a burst of `n` cost O(n²) — measured at 362ms of pure + /// Sweeping on *every* append instead would cost `n` atomic + /// `weak_ptr::expired()` loads under `_pendingMtx` to admit one call with + /// `n` in flight, so a burst of `n` costs O(n²) — measured at 362ms of pure /// admission time for 32k queued executes against one slow model, against /// 7.6ms without the sweep. /// @@ -1565,7 +1557,7 @@ class LocalBackend : public detail::IBackend { // Every live instance, private and shared alike, plus the shared-instance // directory over them — holder, attach count, directory key and hydration // state as one record per instance rather than five parallel ModelId-keyed - // maps held in lockstep by convention (morph#523). Guarded by `_regMtx`; + // maps held in lockstep by convention. Guarded by `_regMtx`; // `InstanceDirectory` is caller-locked by design, see its doc comment. detail::InstanceDirectory _instances; // Ids of models whose holder answered `isBackendChangeAware() == true` at @@ -1585,13 +1577,12 @@ class LocalBackend : public detail::IBackend { static constexpr std::size_t kPendingCompactFloor = 32; mutable std::mutex _pendingMtx; // Sinks, not completion states: a dispatch's settle point is whatever the - // caller handed down, which for `Bridge` is its own typed completion state - // (morph#572, Part B). A `weak_ptr` still, for the same reason as before -- - // this list must not keep a finished dispatch alive. + // caller handed down, which for `Bridge` is its own typed completion state. + // A `weak_ptr`, so this list cannot keep a finished dispatch alive. std::vector> _pending; // Size at which `trackPending` next sweeps `_pending` for expired entries; // re-armed at twice the surviving count after each sweep. Guarded by - // `_pendingMtx` along with `_pending` itself. See `trackPending` (morph#528). + // `_pendingMtx` along with `_pending` itself. See `trackPending`. std::size_t _compactAt = kPendingCompactFloor; // Concurrent in-flight executes, for the executeInFlight metric. A // shared_ptr (not a plain atomic member) so strand tasks hold their own diff --git a/include/morph/core/bridge.hpp b/include/morph/core/bridge.hpp index 6535cb2e..d79c3738 100644 --- a/include/morph/core/bridge.hpp +++ b/include/morph/core/bridge.hpp @@ -99,7 +99,7 @@ class ActionExecuteRegistry { void* handler, std::string_view bodyJson) const { // See `ActionDispatcher::dispatch` -- the registration-phase latch, // closed on the first read of a process-level registry so a later - // registration can assert (morph#698). Debug builds only. + // registration can assert. Debug builds only. ::morph::model::detail::noteRegistryRead(this == &instance()); auto iter = _executors.find( KeyView{.modelId = modelId, .actionId = actionId, .sharing = std::type_index{typeid(Sharing)}}); @@ -126,8 +126,8 @@ class ActionExecuteRegistry { // The key a caller looks an entry up *with*. Every id reaching `execute` // arrives as a `string_view` -- a schema-driven GUI's decoded action name, // or a `constexpr` `ModelTraits::typeId()` -- so materialising `Key` - // just to hash it charged every `executeJson` two `std::string` - // constructions (morph#699). Deliberately not + // just to hash it would charge every `executeJson` two `std::string` + // constructions. Deliberately not // `morph::model::detail::PairKeyView`: this key carries a `std::type_index` // as well as the two ids, so it needs its own view type and its own // functors rather than a reuse that would silently drop the sharing tag. @@ -163,9 +163,8 @@ class ActionExecuteRegistry { struct KeyEqual { // Marks the functor transparent, enabling heterogeneous lookup. A // transparent hash alone is not enough: `unordered_map` requires both - // (morph#699, which is the trap `journal::PayloadMigrationRegistry` - // fell into by naming a transparent hash and keeping the default - // equality). + // before a heterogeneous `find` compiles, and naming only the hash + // leaves every lookup silently materialising a `Key`. using is_transparent = void; // Accepts any mix of `Key` and `KeyView` on either side. @@ -194,20 +193,19 @@ inline bool registerActionExecutorOnce(std::string_view modelId, std::string_vie // Applying the check's own fix does not compile: `static_assert` on this // condition is "static assertion expression is not an integral constant // expression / non-constexpr function 'registrationPhaseClosed' cannot be used - // in a constant expression". Reported upstream of this repository in morph#742. + // in a constant expression". // // Last re-checked at clang-tidy 22.1.8, the version CI pins: the diagnostic // still fires and the suggested fix still does not compile. The stamp is here - // because nothing checks it for you -- morph#755 deleted both - // scripts/check_nolint_directives.sh (which would have flagged a directive that - // had stopped suppressing anything) and scripts/check_ci_clang_pin.sh (the - // natural re-check trigger on a CLANG_VERSION bump). Re-read this when the pin - // moves; if the finding is gone, delete all four directives together. + // because nothing re-checks it for you -- no script flags a directive that has + // stopped suppressing anything, and nothing triggers a re-read on a + // CLANG_VERSION bump. Re-read this when the pin moves; if the finding is gone, + // delete all four directives together. // NOLINTNEXTLINE(misc-static-assert,cert-dcl03-c) assert(!::morph::model::registrationPhaseClosed() && "registerActionExecutorOnce: registration after the registration phase closed. The " "process-level registries are unsynchronised and are read-only once dispatch begins -- " - "registering now races their internals against concurrent lookups (morph#698; " + "registering now races their internals against concurrent lookups (see " "docs/spec/core/registry.md, \"Thread safety\"). Load and register plugin modules before " "the first dispatch."); ::morph::bridge::ActionExecuteRegistry::instance().registerAction(modelId, actionId); @@ -411,8 +409,8 @@ struct AsyncDispatchHandoff { /// `false` if the caller owns the outcome and should deliver it itself. /// /// @par Reachability of the double-claim arm -/// No backend can reach it today, and that is a property of the callers rather -/// than of this function (morph#648). Every one of the eight call sites below +/// No backend can reach it, and that is a property of the callers rather +/// than of this function. Every one of the eight call sites below /// is a `.then`/`.onError` on one `Completion`, and a `CompletionState` settles /// once — the second `resolve`/`reject` is a documented no-op — so exactly one /// of the two lambdas runs, exactly once, and `fired` is always `false` on @@ -431,9 +429,9 @@ inline bool parkIfInFrame(AsyncDispatchHandoff& handoff, bool succeeded, ::morph if (handoff.fired) { // A backend is contractually allowed exactly one callback per dispatch; // swallow a second one rather than reporting twice. Unreachable from - // a backend since morph#571 put every dispatch behind one - // `Completion` -- see @par Reachability above for what that rests on - // and why the arm stays. + // a backend, because every dispatch goes behind one `Completion` -- + // see @par Reachability above for what that rests on and why the arm + // stays. return true; } handoff.fired = true; @@ -501,12 +499,12 @@ inline std::optional awaitHandoff(AsyncDispatchHandoff& handoff) /// /// The counterpart to `parkIfInFrame` returning `false`: nobody is left on the /// dispatching stack to publish this outcome, so the callback publishes it -/// itself — and, until morph#588, published it on whichever thread the backend +/// itself. With a null @p exec it publishes on whichever thread the backend /// happened to settle the `Completion` on, because the executor every dispatch -/// site names is `exec::detail::inlineExecutor()`. That is the thread the -/// morph#486 use-after-free is about: each of these callbacks asks "is the -/// `Bridge` still alive" and then touches it, and a `~Bridge` running -/// concurrently on another thread can land between the two steps. +/// site names is `exec::detail::inlineExecutor()`. That thread is where the +/// use-after-free lives: each of these callbacks asks "is the `Bridge` still +/// alive" and then touches it, and a `~Bridge` running concurrently on another +/// thread can land between the two steps. /// /// Routing only the late delivery through an executor the `Bridge` was given /// closes that window structurally for an embedder whose executor runs tasks @@ -520,18 +518,18 @@ inline std::optional awaitHandoff(AsyncDispatchHandoff& handoff) /// A template rather than a `std::function` parameter, and that is not /// incidental: with a null @p exec the callable is invoked **in place**, so the /// default path type-erases nothing and allocates nothing. Taking a -/// `std::function` would have put a heap allocation — sometimes a large one, -/// since these closures carry a primary key — on the path that existed before -/// morph#588, which `tests/test_async_registration.cpp`'s morph#108 -/// allocation-failure case detects by catching the wrong allocation. +/// `std::function` would put a heap allocation — sometimes a large one, since +/// these closures carry a primary key — on that inline path; +/// `tests/test_async_registration.cpp`'s allocation-failure case detects such a +/// regression by catching the wrong allocation. /// /// @tparam Action Callable of no arguments; convertible to `std::function` only /// on the posting path. /// @param exec Executor to deliver on. Borrowed, and may be null, which is /// the default a `Bridge` constructed without one carries: a -/// null @p exec runs @p action inline, exactly where it ran -/// before morph#588. Non-null, it must outlive every in-flight -/// registration, because a reply can land after `~Bridge`. +/// null @p exec runs @p action inline, on the settling thread. +/// Non-null, it must outlive every in-flight registration, +/// because a reply can land after `~Bridge`. /// @param action Work to run. Must be safe to run after `~Bridge` — every /// caller here gates on a `CallbackToken` or a /// `detail::BridgeLifetime` before touching the bridge. @@ -554,10 +552,10 @@ void deliverLate(::morph::exec::IExecutor* exec, Action&& action) { /// simply not run and the check being stale costs nothing. It is *not* enough /// to gate a **member call on the `Bridge`**: the bridge can be destroyed in /// the instructions between the check and the call, and the call then runs on -/// destroyed memory. That is issue #486 — a `~BridgeHandler` running on a -/// worker thread saw an active token, and `Bridge::deregisterHandler` then -/// iterated a `_handlers` vector whose `Bridge` the owning thread had already -/// finished destroying. +/// destroyed memory. Concretely, a `~BridgeHandler` running on a worker thread +/// sees an active token, and `Bridge::deregisterHandler` then iterates a +/// `_handlers` vector whose `Bridge` the owning thread has already finished +/// destroying. /// /// This type closes that window structurally rather than per call site: the /// answer is only ever read while `mtx` is held, and `~Bridge` flips it while @@ -595,41 +593,30 @@ struct BridgeLifetime { /// @brief The typed completion state a backend settles directly. /// -/// `Bridge::executeVia` used to create two completions per dispatch: the typed -/// one it hands the caller, and the erased -/// `Completion>` the backend produced, with a `.then` / -/// `.onError` pair forwarding one into the other. Everything the forwarding -/// block did — disarm the deadline, decrement `_pendingCalls`, run `onResult` -/// and publish under one liveness snapshot, guard the value move — is a method -/// on this class instead, and the backend settles it through `ISettleSink`. -/// Six allocations per call become one (morph#572, Part B): +/// This object *is* the typed completion state `Bridge::executeVia` hands the +/// caller, and is also the `ISettleSink` the backend settles. One object rather +/// than a typed completion plus an erased `Completion>` +/// with a `.then`/`.onError` pair forwarding between them, which costs one +/// allocation per dispatch instead of six — the two states, the two forwarding +/// closures, and their two handler vectors. /// -/// | what | before | after | -/// |---|---|---| -/// | typed `CompletionState` | 1 | 1 (this object) | -/// | erased `CompletionState>` | 1 | — | -/// | `.then` / `.onError` forwarding closures | 2 | — | -/// | their two handler vectors | 2 | — | -/// -/// @par What had to be preserved, and where it now lives -/// Each of these carries a comment naming the bug it came from; moving them -/// was the risk in this change, so they are enumerated rather than left to be -/// rediscovered. -/// - **The deadline disarm happens first**, before any forwarding work, so a +/// @par The four invariants the settle path holds +/// Each is a property of *this* class's settle methods, and each is +/// load-bearing on its own. +/// - **The deadline disarm happens first**, before any other settle work, so a /// slow `onResult`/`publishResult` cannot give the timer a window to resolve /// this completion with `ClientTimeoutError` while the real result is in -/// hand — `settleOnce`, called at the top of both settle methods (morph#620). +/// hand — `settleOnce`, called at the top of both settle methods. /// - **`_pendingCalls` is decremented on exactly one of two mutually exclusive -/// paths**, whether or not the forwarding that follows then throws — -/// `settleOnce` again, which is now also what makes a `cancelPending` -/// racing a reply decrement once rather than twice (morph#489). +/// paths**, whether or not the work that follows then throws — `settleOnce` +/// again, which is also what makes a `cancelPending` racing a reply decrement +/// once rather than twice. /// - **`lifetime->alive` is read once** under the gate and that one snapshot -/// decides both `onResult` and `publishResult`, so the two cannot disagree -/// (morph#486/#489). +/// decides both `onResult` and `publishResult`, so the two cannot disagree. /// - **The value forwarding is guarded**, so a throwing move of `R` routes to /// this state's own error sink instead of escaping the callback executor, /// where `ThreadPoolExecutor` swallows it (hanging the completion) and -/// `QtExecutor` lets it reach the event loop (morph#502). +/// `QtExecutor` lets it reach the event loop. /// /// @par Lifetime /// Everything this holds is pinned: `_pendingCalls`, `_subscriptions` and @@ -682,9 +669,9 @@ class BridgeSink final : public ::morph::async::detail::CompletionState, /// @brief Undoes `armDeadline` and the pending count for a dispatch that /// never started, because `IBackend::executeInto` threw. /// - /// The `morph#502` path: a throw out of the backend left `_pendingCalls` - /// inflated — and `pendingCalls()` is documented as a quiescence gate — and - /// the timer entry stranded. Routed through `settleOnce` so it cannot + /// Without this, a throw out of the backend leaves `_pendingCalls` inflated + /// — and `pendingCalls()` is documented as a quiescence gate — and the timer + /// entry stranded. Routed through `settleOnce` so it cannot /// double-count against a sink the backend had already settled before it /// threw. void abandon() { @@ -727,9 +714,8 @@ class BridgeSink final : public ::morph::async::detail::CompletionState, // path is non-blocking by construction (its doc comment: it prefers // the backend's async registration precisely to avoid a // nested-event-loop block), so holding the gate across it does not - // expose `~Bridge` to the unbounded-block hazard morph#489 names - // for the sites this does not mechanically apply to - // (installReconnectHandler, site 4). + // expose `~Bridge` to the unbounded-block hazard that a guarded + // region calling into consumer-supplied code would carry. bool bridgeAlive = false; { std::shared_lock const gate{_lifetime->mtx}; @@ -805,7 +791,7 @@ class BridgeSink final : public ::morph::async::detail::CompletionState, // A deadline callback already mid-flight still runs its // `setException`, which this state discards as an already-ready // one -- first result wins. That, not the disarm, is what makes - // the race harmless (morph#620). + // the race harmless. } } return true; @@ -841,14 +827,14 @@ class Bridge { /// (e.g. `QtWebSocketBackend`) can ask the bridge to re-register every live /// handler against the freshly reconnected peer. /// - /// @par The bridge's own executor (morph#588) + /// @par The bridge's own executor /// Every other completion in the framework is delivered on an executor its /// caller named — `BridgeHandler` supplies `guiExec`, `executeVia` takes a - /// `cbExec`. The registrations the bridge issues *on its own behalf* had no - /// such executor: their five dispatch sites name + /// `cbExec`. The registrations the bridge issues *on its own behalf* have no + /// such caller: their five dispatch sites name /// `exec::detail::inlineExecutor()`, which is "deliver wherever the backend /// settled" written as a value rather than as a sentence in a doc comment. - /// @p bridgeExec is where that decision now lives. + /// @p bridgeExec is where that choice lives. /// /// It is used for **one** thing: a registration reply that arrives after /// its dispatching frame has gone (`detail::deliverLate`). That is the only @@ -871,15 +857,15 @@ class Bridge { /// **What the caller must guarantee, and what it buys.** @p bridgeExec must /// outlive this bridge and every registration still in flight when it is /// destroyed, because a late reply can land after `~Bridge` (the same - /// requirement `BridgeHandler`'s `guiExec` already carries). The window - /// morph#486 describes is closed only if @p bridgeExec runs its tasks on a - /// thread that cannot run `~Bridge` concurrently — for a Qt embedder, the + /// requirement `BridgeHandler`'s `guiExec` already carries). The + /// check-then-touch window is closed only if @p bridgeExec runs its tasks + /// on a thread that cannot run `~Bridge` concurrently — for a Qt embedder, the /// GUI thread that both owns the `Bridge` and pumps the executor. Supplying /// an executor on some *other* thread satisfies the type and does not close /// the window; it is not made worse than the default either, since the - /// callbacks' existing `CallbackToken`/`detail::BridgeLifetime` gates are - /// unchanged. Left null — the default — delivery is inline and behaviour is - /// byte-for-byte what it was before morph#588. + /// callbacks' `CallbackToken`/`detail::BridgeLifetime` gates still apply. + /// Left null — the default — delivery is inline, on whichever thread the + /// backend settled the reply. /// /// @param backend Initial backend. Ownership is transferred. /// @param bridgeExec Executor for late registration continuations, or null @@ -917,7 +903,7 @@ class Bridge { /// otherwise still be walking. `closeLifetime()` both publishes "this bridge is /// retired" and **waits out** any deregistration already inside the gate, so /// this destructor never overlaps one. That wait is bounded and cannot - /// deadlock: see `detail::BridgeLifetime`. Issue #486. + /// deadlock: see `detail::BridgeLifetime`. ~Bridge() { closeLifetime(); if (auto active = loadBackend()) { @@ -1031,22 +1017,19 @@ class Bridge { /// helper a second mode, and the two behaviours are different on purpose. /// /// @par The liveness check and the `this` touch are two steps - /// That is the morph#486 shape, and it is closed not by a gate but by the - /// thread this body runs on. Before morph#571 that was a prose contract on - /// every backend author; morph#568 made it the executor the dispatch site - /// names, which is `inlineExecutor()` and so left the window unchanged; - /// morph#588 moved the choice to the bridge's own executor, where a - /// non-null one running `~Bridge`'s thread closes it, and the null default - /// keeps the pre-morph#588 thread exactly. Gating instead would block - /// `~Bridge` behind `_attachMtx`, which `attachHandler` holds across a full - /// `attachModel` round trip. See morph#489. + /// That window is closed not by a gate but by the thread this body runs on, + /// which is the bridge's own executor: a non-null one running `~Bridge`'s + /// thread closes it outright, and the null default delivers inline on + /// whichever thread the backend settled the reply. Gating instead would + /// block `~Bridge` behind `_attachMtx`, which `attachHandler` holds across + /// a full `attachModel` round trip. /// /// @par Locking /// @p publish runs under `_attachMtx`, because `contextKey`/`primary` are /// plain `std::string`s that every other site reads under that lock -- /// publishing them without it would be a data race, not merely a stale /// read. (`registerHandlerImpl`'s read during registration is the one - /// documented carve-out; see its own comment, and morph#505.) @p onDone is + /// documented carve-out; see its own comment.) @p onDone is /// invoked **after** the lock is released, on every path that invokes it at /// all: what a caller does from inside it is dispatch the action, which can /// re-enter `_attachMtx` through `assignHandlerPrimary`. @@ -1110,7 +1093,7 @@ class Bridge { /// invokes @p onDone once attached (or failed), instead of blocking. /// /// Reaches the backend through `IBackend::bindModel` — the structural - /// registration surface, and since morph#571 the only one. There is + /// registration surface, and the only one. There is /// exactly one dispatch and exactly one continuation: a /// backend with no non-blocking attach settles inside the `bindModel` call, /// having blocked for the same round trip the synchronous `attachHandler` @@ -1197,15 +1180,15 @@ class Bridge { // `mutable`, so `primaryCopy` is *moved* into the body's closure // rather than copied: this callback runs exactly once (one // `Completion`, settled once), and copying the primary key here - // would put a fresh allocation on a path that had none before - // morph#588 -- which is both a cost and, for - // `tests/test_async_registration.cpp`'s morph#108 case, the wrong - // allocation for its injector to catch. `onDone` cannot be moved + // would put a fresh allocation on a path that has none -- which is + // both a cost and, for `tests/test_async_registration.cpp`'s + // allocation-failure case, the wrong allocation for its injector to + // catch. `onDone` cannot be moved // the same way: it is captured from a `const` reference parameter, // so the capture itself is const. detail::deliverLate(bridgeExec, [this, weakBackend, weakLiveness, weakBinding, primaryCopy = std::move(primaryCopy), onDone, newId] { - // Guards, locking and the morph#486 reasoning all live in + // Guards, locking and the liveness reasoning all live in // `publishLateBindReply`, which `ensureBoundAsync` shares. // What is this site's own is the three fields a successful // attach publishes -- and that they can throw, which is why @@ -1231,14 +1214,12 @@ class Bridge { // The structural surface (`IBackend::bindModel`). A backend with a // genuinely non-blocking attach settles the returned `Completion` // when its reply lands; one without settles it from inside this - // call, having blocked exactly as the synchronous `attachModel` - // this replaces did. Either way the continuation exists, so there + // call, having blocked for the same round trip the synchronous + // `attachModel` would. Either way the continuation exists, so there // is no second path here. // - // `inlineExecutor()` because that is where the continuation ran - // before morph#568: on whichever thread the backend settled the - // reply on, which the removed `*Async` twins could only ask for in - // prose. What is new is that this call site *names* it; see + // `inlineExecutor()` names, as a value, "deliver on whichever + // thread the backend settled the reply on"; see // `exec::detail::InlineExecutor`. An inline settle therefore // reaches `onAttached`/`onFailed` while `_attachMtx` is still // held, which is precisely the case `handoff` exists for. @@ -1414,7 +1395,7 @@ class Bridge { /// wire backends, a reply field) — tracked as a follow-up, not fixed /// here. /// Reaches the backend through `IBackend::promoteModel` — the structural - /// registration surface, and since morph#571 the only one. + /// registration surface, and the only one. /// The same "avoid a nested-event-loop block that aborts a WASM /// main thread" rationale `Bridge::registerHandler()` follows for the /// initial bind step applies here: this method is invoked from inside the @@ -1455,17 +1436,12 @@ class Bridge { std::weak_ptr<::morph::backend::detail::IBackend> const weakBackend{backend}; std::weak_ptr const weakBinding{binding}; auto onPromoted = [this, weakLiveness, weakBackend, weakBinding, primary] { - // This check and the `this` touch below it are two steps -- the - // morph#486 shape. Closed not by a gate but by the thread this - // body runs on. Before morph#571 that thread was the backend's - // choice, asked for in prose; morph#568 made it the executor the - // `promoteModel` call names, which is `inlineExecutor()` and so - // left the window unchanged; morph#588 moved the choice to the - // bridge's own executor for a reply that arrives after this - // frame, and left the in-frame reply published by this frame. + // This check and the `this` touch below it are two steps. That + // window is closed not by a gate but by the thread this body runs + // on: a reply arriving after this frame goes to the bridge's own + // executor, and an in-frame reply is published by this frame. // Gating instead would block `~Bridge` behind `_attachMtx`, which // `attachHandler` holds across a full `attachModel` round trip. - // See morph#489. if (!weakLiveness.active()) { return; // The Bridge is gone; do not touch `this`. } @@ -1497,19 +1473,18 @@ class Bridge { }; // The structural surface (`IBackend::promoteModel`). A backend with // no non-blocking promote settles the returned `Completion` from - // inside this call, having run the same synchronous `assignPrimary` - // this replaces; `inlineExecutor()` then delivers the reply on this - // thread, and the `claimHandoff` below publishes it before this - // method returns -- exactly where the synchronous call used to - // publish, which `BridgeHandler::execute`'s `onResult` relies on ("the - // binding is already promoted by the time user code sees the result"). - // Unlike that call, a failure is logged rather than thrown: this runs - // inside the result `Completion`'s callback chain, where an escaping - // exception is swallowed by `CompletionState` anyway, and - // `promoteModel` reports through the `Completion` by contract. + // inside this call, having run the synchronous `assignPrimary`; + // `inlineExecutor()` then delivers the reply on this thread, and the + // `claimHandoff` below publishes it before this method returns, which + // `BridgeHandler::execute`'s `onResult` relies on ("the binding is + // already promoted by the time user code sees the result"). A failure + // is logged rather than thrown: this runs inside the result + // `Completion`'s callback chain, where an escaping exception is + // swallowed by `CompletionState` anyway, and `promoteModel` reports + // through the `Completion` by contract. // - // The handoff is what separates the two cases morph#588 treats - // differently, and is why this site has one at all: a reply that + // The handoff is what separates the two delivery cases, and is why + // this site has one at all: a reply that // lands in this frame is published by this frame, while one that // lands later goes to the bridge's executor. Without it the callback // could not tell the two apart, and posting *both* would make an @@ -1861,7 +1836,7 @@ class Bridge { // below builds a single `register`/`registerShared` envelope per live // binding — otherwise every control envelope re-registering handlers on // the new backend would carry a default-constructed (unauthenticated) - // session, exactly the #63 gap this hook closes. Read under + // session, which is the gap this hook closes. Read under // `_sessionMtx` alone (a leaf mutex never held while calling into this // backend), mirroring `executeVia`'s copy-then-release pattern. { @@ -1879,10 +1854,7 @@ class Bridge { // backend that answers `kCallerMustNotBlock` delivers its replies // through the calling thread's own event loop, so waiting here is a // deadlock rather than a delay — on a WASM main thread, a page abort. - // Until morph#615 this site did not ask at all: it called the blocking - // `registerModelShared`/`registerModelWithContext` directly, so a - // backend that had just been given a way to say "do not do this to me" - // was blocked here anyway. See `IBackend::bindWaitPolicy`. + // See `IBackend::bindWaitPolicy`. bool const mayBlock = newShared->bindWaitPolicy() == ::morph::backend::detail::BindWait::kCallerMayBlock; { // Both mutexes: this phase reads/writes every live binding's @@ -1910,9 +1882,9 @@ class Bridge { if (staging) { // Rethrown here rather than from inside the locked block, so the // waiters above are settled outside `_mtx`/`_attachMtx`. Everything - // below stays unreached on this path, exactly as before: a staging - // failure leaves the outgoing backend's reconnect handler and its - // pending completions alone, because the switch did not happen. + // below stays unreached on this path: a staging failure leaves the + // outgoing backend's reconnect handler and its pending completions + // alone, because the switch did not happen. std::rethrow_exception(staging); } if (previous && previous != newShared) { @@ -1995,12 +1967,11 @@ class Bridge { auto backend = loadBackend(); uint64_t const raw = binding->currentId.load(); - // One allocation where there used to be six: this object *is* the - // typed completion state the caller gets, and is also the - // `ISettleSink` the backend settles -- so there is no second, erased - // completion and no `.then`/`.onError` pair forwarding one into the - // other. See `detail::BridgeSink` for what the forwarding block did and - // where each piece of it went (morph#572, Part B). + // One allocation, not six: this object *is* the typed completion state + // the caller gets, and is also the `ISettleSink` the backend settles -- + // so there is no second, erased completion and no `.then`/`.onError` + // pair forwarding one into the other. See `detail::BridgeSink` for the + // four invariants its settle path holds. auto sink = std::make_shared>(::morph::exec::detail::ModelId{raw}, std::move(onResult), _pendingCalls, _subscriptions, _lifetime); ::morph::async::Completion typed{sink, cbExec}; @@ -2051,13 +2022,11 @@ class Bridge { // mid-flight when the real reply lands still runs its // `setException`, which `CompletionState` discards on an // already-ready state -- first result wins. That, not the - // disarm, is what makes the race harmless (morph#620). + // disarm, is what makes the race harmless. // The callback settles the *state*, not the sink: a deadline // that fires is not one of the two mutually-exclusive // resolution paths and must not decrement `_pendingCalls`, - // which stays inflated until the real reply lands. That is - // unchanged from the forwarding shape, where the timer - // likewise reached `typedState` directly. + // which stays inflated until the real reply lands. auto const handle = schedulerRef->schedule(_executeDeadline, [sink] { sink->setException(std::make_exception_ptr(::morph::backend::ClientTimeoutError{})); }); @@ -2070,13 +2039,13 @@ class Bridge { ::morph::backend::detail::ActionCall call; // Views of `constexpr` string literals, and stateless operations // addressed rather than copied: this whole block allocates exactly - // once now (the action itself), where it used to allocate four times - // -- two `std::string` copies of compile-time constants and two - // `std::function`s whose `shared_ptr` capture defeats libstdc++'s - // small-object buffer -- on every call, including the `LocalBackend` - // calls that never look at `serializeAction` or `deserializeResult`. - // See `backend::detail::ActionCall` for the lifetime contract this - // shape carries and morph#572 for the measurement. + // once (the action itself). Copying instead would cost four + // allocations per call -- two `std::string` copies of compile-time + // constants and two `std::function`s whose `shared_ptr` capture + // defeats libstdc++'s small-object buffer -- including on the + // `LocalBackend` calls that never look at `serializeAction` or + // `deserializeResult`. See `backend::detail::ActionCall` for the + // lifetime contract this shape carries. call.modelTypeId = ::morph::model::ModelTraits::typeId(); call.actionTypeId = ::morph::model::ActionTraits::typeId(); auto sharedAction = std::make_shared(std::move(action)); @@ -2180,8 +2149,7 @@ class Bridge { // `BridgeHandler::execute` with `_pendingCalls` already incremented and // the deadline already armed, permanently inflating `pendingCalls()` -- // which the class documents as a quiescence gate -- and stranding the - // timer entry. Undo both, then let the exception continue to the caller - // (morph#502). + // timer entry. Undo both, then let the exception continue to the caller. try { backend->executeInto(::morph::exec::detail::ModelId{raw}, std::move(call), cbExec, sink); } catch (...) { @@ -2204,14 +2172,14 @@ class Bridge { /// Note `~BridgeHandler` does **not** use this: gating a *call into* the /// bridge needs `lifetimeGate()`'s `detail::BridgeLifetime`, because a token /// answers only advisorily and a member call made a few instructions after a - /// stale "active" runs on destroyed memory (morph#486). A token is the right + /// stale "active" runs on destroyed memory. A token is the right /// tool for *declining work*, not for keeping an object alive across a call. /// The bridge must still outlive its handlers for normal `execute`/`set` /// calls; the gate only makes *teardown* order-independent. /// - /// The bridge is the framework's own first consumer of the primitive every - /// caller now gets (docs/spec/core/callback_scope.md). It uses only the - /// liveness half: `_callbacks` is never stopped explicitly, so its tokens go + /// The bridge uses only the liveness half of the primitive + /// (docs/spec/core/callback_scope.md): `_callbacks` is never stopped + /// explicitly, so its tokens go /// inactive exactly when the `Bridge` is destroyed. [[nodiscard]] ::morph::async::CallbackToken liveness() const { return _callbacks.token(); } @@ -2242,15 +2210,15 @@ class Bridge { /// @brief Shared body of both `registerHandler()` overloads: binds through /// `IBackend::bindModel`, the structural registration surface, and - /// since morph#571 the only one. + /// the only one. /// /// A backend with a non-blocking bind returns an unsettled `Completion` and /// the binding is returned unbound (see `IBackend::bindModel`'s doc comment /// for why that matters — a nested-event-loop block aborts a WASM main /// thread). One with only the blocking default settles inside the call, - /// having run exactly the `registerModelWithContext` this used to call - /// directly, so the binding is bound before this returns and a failure is - /// **rethrown** to the caller, as that call used to throw. + /// having run the blocking `registerModelWithContext` the request's shape + /// names, so the binding is bound before this returns and a failure is + /// **rethrown** to the caller. /// /// @p binding is added to `_handlers` *before* the backend call, so a /// concurrently-running `switchBackend()`/reconnect can already see and @@ -2296,7 +2264,7 @@ class Bridge { // the caller the binding first, so the requirement is on the caller: // **set `contextKey` before calling `registerHandler()`, and do not // mutate it concurrently with that call.** After registration returns, - // every access goes under `_attachMtx` as documented. morph#505. + // every access goes under `_attachMtx` as documented. // // Both continuations come from `makeBindCallbacks`, shared with // `switchBackend`'s phase 1 and the reconnect handler: a stale reply @@ -2308,13 +2276,12 @@ class Bridge { // The structural surface (`IBackend::bindModel`). An empty `primary` // with a zero `current` is the request shape that means - // `registerModelWithContext` -- the verb this branch used to call - // directly -- so a backend with no non-blocking bind runs exactly that, - // blocks exactly as long, and settles before `bindModel` returns. + // `registerModelWithContext`, so a backend with no non-blocking bind + // runs that blocking verb and settles before `bindModel` returns. // - // `inlineExecutor()` because that is where this continuation ran - // before: on whichever thread the backend settled on, which for the - // blocking default is this one. See `exec::detail::InlineExecutor`. + // `inlineExecutor()` delivers the continuation on whichever thread the + // backend settled on, which for the blocking default is this one. See + // `exec::detail::InlineExecutor`. // // A synchronous backend that *fails* must still fail the way it used to // -- `registerModelWithContext` threw out of `registerHandler()`, and a @@ -2338,9 +2305,9 @@ class Bridge { } // Only reachable for a `kCallerMustNotBlock` backend, whose // reply lands after this frame gave up waiting: exactly the - // delivery morph#588 gives the bridge's executor. The - // in-frame outcome below is published by this frame instead, - // on this thread, whatever executor the bridge holds. + // delivery the bridge's own executor exists for. The in-frame + // outcome below is published by this frame instead, on this + // thread, whatever executor the bridge holds. detail::deliverLate(bridgeExec, [registered = std::move(onRegistered), newId] { registered(newId); }); }) .onError([onFailed, handoff, bridgeExec](const std::exception_ptr& failure) mutable { @@ -2363,10 +2330,9 @@ class Bridge { // an exception that matters: a `QtWebSocketBackend` with // `asyncRegistrationEnabled` set delivers its reply through the Qt // event loop of this very thread, so waiting here is a deadlock, not a - // delay -- on a WASM main thread it aborts the page (morph#568). Such a - // backend's caller gets an unbound handler and must gate on - // `whenBound()`, exactly as the removed `*Async` path already required - // of it. See `IBackend::bindWaitPolicy` and morph#593. + // delay -- on a WASM main thread it aborts the page. Such a backend's + // caller gets an unbound handler and must gate on `whenBound()`. See + // `IBackend::bindWaitPolicy`. auto parked = backend->bindWaitPolicy() == ::morph::backend::detail::BindWait::kCallerMayBlock ? detail::awaitHandoff(*handoff) : detail::claimHandoff(*handoff); @@ -2403,8 +2369,8 @@ class Bridge { // Only the failure callback above supplies an `err`; the // success callback settles through this same arm with a null // one whenever the reply's id was discarded (`applied` false) - // and the binding is still unbound. `CompletionState` now - // refuses to settle on a null (issue #347), but it can only + // and the binding is still unbound. `CompletionState` refuses + // to settle on a null, but it can only // substitute a generic message — the meaning of *this* // failure is known here and nowhere else, so name it here. onErr(err ? err @@ -2428,9 +2394,9 @@ class Bridge { /// The success continuation holds `lifetime`'s gate across the whole touch /// of `this` (`_mtx`, `loadBackend()`) rather than only checking at entry — /// `CallbackToken::active()` is advisory and cannot carry that weight, see - /// `detail::BridgeLifetime`. Holding it across this span is safe for the - /// reason morph#489 gives: nothing inside is a call into unbounded or - /// consumer-supplied code, only a mutex and a backend-pointer comparison. + /// `detail::BridgeLifetime`. Holding it across this span is safe because + /// nothing inside is a call into unbounded or consumer-supplied code, only + /// a mutex and a backend-pointer comparison. /// The id is published only while @p backend is still the active one, so a /// reply from a backend `switchBackend()` has already replaced cannot /// overwrite the id that switch just installed. @@ -2485,13 +2451,12 @@ class Bridge { /// `IBackend::bindModel`, waiting only if @p mayBlock says it may. /// /// The one dispatch shape `switchBackend`'s staging phase and the reconnect - /// handler share (morph#615). A non-empty `primary` with a zero `current` - /// is the request shape that means `registerModelShared`, an empty one the - /// shape that means `registerModelWithContext` — the two blocking verbs - /// both sites called directly until morph#615 — so a backend with only the - /// default `bindModel` runs exactly the call it always did and settles - /// before this returns. `inlineExecutor()` because that is where these - /// continuations ran before; see `attachHandlerAsync`. + /// handler share. A non-empty `primary` with a zero `current` is the request + /// shape that means `registerModelShared`, an empty one the shape that means + /// `registerModelWithContext`, so a backend with only the default + /// `bindModel` runs the corresponding blocking verb and settles before this + /// returns. `inlineExecutor()` delivers on the settling thread; see + /// `attachHandlerAsync`. /// /// A reply that lands inside this frame is parked rather than acted on, /// because both callers hold `_mtx`/`_attachMtx` here and the continuation @@ -2523,8 +2488,8 @@ class Bridge { } // A deferred reply, so both callers have long since released // `_mtx`/`_attachMtx` that this continuation re-takes; it is - // also the delivery whose thread morph#588 lets the bridge - // choose. See `detail::deliverLate`. + // also the delivery whose thread the bridge's own executor + // chooses. See `detail::deliverLate`. detail::deliverLate(bridgeExec, [bound = std::move(onBound), newId] { bound(newId); }); }) .onError([onFailed, handoff, bridgeExec](const std::exception_ptr& failure) mutable { @@ -2586,8 +2551,7 @@ class Bridge { } // Only armed on the path that can actually defer — a waiting // frame settles every outcome itself, so `registrationInFlight` - // stays what it was before morph#615 for every backend that - // lets this frame wait. + // is never armed for a backend that lets this frame wait. if (!mayBlock) { armRegistration(*binding); out.armed.push_back(binding); @@ -2599,10 +2563,10 @@ class Bridge { continue; } if (!parked->succeeded) { - // A rejected `Completion`, where this loop used to catch a - // throw: the structural surface reports failure through the - // completion, and a bare `catch (...)` can no longer see - // it. The rollback keys on this (morph#615). + // A rejected `Completion`: the structural surface reports + // failure through the completion, not by throwing, so a + // bare `catch (...)` cannot see it. The rollback keys on + // this. return parked->failure; } out.staged.emplace_back(binding, parked->modelId.v); @@ -2717,11 +2681,10 @@ class Bridge { continue; } if (!parked->succeeded) { - // Before morph#615 a failing re-registration threw out of the - // handler and onto the transport thread, taking every binding - // after it with it. A rejected `Completion` is reported per - // binding instead: this one is left unbound and the loop - // carries on. + // A rejected `Completion` is reported per binding: this one is + // left unbound and the loop carries on, rather than throwing + // out of the handler onto the transport thread and taking + // every binding after it along. binding->currentId.store(0); settle.emplace_back(binding, parked->failure); continue; @@ -2776,9 +2739,9 @@ class Bridge { // backend's *transport* thread, and for a `QtWebSocketBackend` // with `asyncRegistrationEnabled` that thread is the one whose // event loop has to deliver the reply. Calling the blocking - // verb here — which is what this loop did until morph#615 — - // parks it against itself, and on a WASM main thread aborts - // the page. See `IBackend::bindWaitPolicy` and morph#568. + // verb here would park that thread against itself, and on a + // WASM main thread abort the page. See + // `IBackend::bindWaitPolicy`. settle = reregisterLive( pinned, pinned->bindWaitPolicy() == ::morph::backend::detail::BindWait::kCallerMayBlock); } @@ -2795,8 +2758,8 @@ class Bridge { mutable std::mutex _backendMtx; std::shared_ptr<::morph::backend::detail::IBackend> _backend; // Where a registration reply that missed its dispatching frame is - // delivered; null means "inline, on the settling thread", which is what - // every site did before morph#588. Read once per dispatch and captured by + // delivered; null means "inline, on the settling thread". Read once per + // dispatch and captured by // value into the continuation, never read *from* the callback: the // callback can run after `~Bridge`, and `this->_bridgeExec` would then be // a read of destroyed memory ahead of the very gate that exists to @@ -2820,10 +2783,10 @@ class Bridge { // registration*, because acquiring `_attachMtx` there would make // `registerHandler()` contend with a slow shared attach — the exact // regression "Bridge: an in-flight shared attach does not block unrelated - // handler registration" (tests/test_shared_instances.cpp) was written to + // handler registration" (tests/test_shared_instances.cpp) is written to // catch, and which taking the lock there demonstrably reproduces. That read // is ordered rather than locked; see its own comment for the requirement - // that places on a caller of the pre-built-binding overload (morph#505). `switchBackend()` and the reconnect + // that places on a caller of the pre-built-binding overload. `switchBackend()` and the reconnect // handler, which also touch them alongside `_handlers`, take both mutexes together via `std::scoped_lock{_mtx, // _attachMtx}` (deadlock-safe regardless of acquisition order, by `std::scoped_lock`'s own guarantee). std::mutex _attachMtx; @@ -2848,13 +2811,12 @@ class Bridge { // // Heap-allocated and shared, like `_lifetime` below: `executeVia()`'s // `.then` continuation needs to call `hasSubscribers()`/`publishResult()` - // from a possibly-post-~Bridge() context (morph#489, sites 1/2), and - // `SubscriptionRegistry` already snapshots its sinks under its own lock - // and invokes them outside it (see that class), so pinning the registry - // itself with a captured `shared_ptr` -- rather than gating a touch of - // `this` -- makes the call safe with no risk of blocking `~Bridge` behind - // a subscriber's own callback (the deadlock class morph#489 names for the - // sites this fix does *not* mechanically apply to). Never null. + // from a possibly-post-~Bridge() context, and `SubscriptionRegistry` + // already snapshots its sinks under its own lock and invokes them outside + // it (see that class), so pinning the registry itself with a captured + // `shared_ptr` -- rather than gating a touch of `this` -- makes the call + // safe with no risk of blocking `~Bridge` behind a subscriber's own + // callback. Never null. std::shared_ptr> _subscriptions{ std::make_shared>()}; // Count of executeVia() dispatches not yet resolved -- see pendingCalls(). @@ -2877,7 +2839,7 @@ class Bridge { // able to *ask* whether the bridge is there, so the answer cannot live in // the Bridge's own storage. Declaration position is irrelevant for the same // reason -- `~Bridge`'s first statement retires it explicitly, long before - // any member is destroyed. See detail::BridgeLifetime and issue #486. + // any member is destroyed. See detail::BridgeLifetime. std::shared_ptr _lifetime{std::make_shared()}; }; @@ -2968,8 +2930,8 @@ class BridgeHandler { /// — a worker-pool thread for `LocalBackend`/`SimulatedRemoteBackend`, the /// transport thread for `SocketBackend`. A bare "is the bridge alive?" check /// answers for an instant that has already passed by the time the call is - /// made, which is how issue #486 turned an ordinary `~App` into a - /// use-after-free on `Bridge::deregisterHandler`'s `_handlers`. Holding the + /// made, which is how an ordinary `~App` becomes a use-after-free on + /// `Bridge::deregisterHandler`'s `_handlers`. Holding the /// shared lock across the call makes the check and the call one step: /// `~Bridge` cannot start until this returns, and a `~Bridge` that started /// first is already visible here as "not alive". @@ -3184,16 +3146,16 @@ class BridgeHandler { [[nodiscard]] ::morph::async::Completion executeJson(std::string_view actionType, std::string_view bodyJson) { // Dispatches through the executor registered for this handler's own - // Sharing policy (see issue #68 / ActionExecuteRegistry::registerAction's - // doc comment): a NoSharing-only executor would static_cast `this` + // Sharing policy (see `ActionExecuteRegistry::registerAction`'s doc + // comment): a NoSharing-only executor would static_cast `this` // to the wrong BridgeHandler instantiation for a // shared handler, silently skipping its attach/promote step. // // `typeId()` is a `constexpr std::string_view` over a string literal, - // so it is passed straight through. It used to be copied into a - // `std::string` here, which allocated once per call for any model id - // past the SSO buffer -- the same allocation, on the same path, that - // the transparent lookup below it removes (morph#699). + // so it is passed straight through. Copying it into a `std::string` + // here would allocate once per call for any model id past the SSO + // buffer -- the same allocation, on the same path, that the transparent + // lookup below it avoids. return ActionExecuteRegistry::instance().execute(::morph::model::ModelTraits::typeId(), actionType, this, bodyJson); } @@ -3325,8 +3287,8 @@ class BridgeHandler { /// Builds one executor per `Sharing` policy the framework defines /// (`NoSharing`, `AllowShared`) from the same generic-lambda template, /// `static_cast`ing `handlerVoid` to the matching `BridgeHandler*` in each — see issue #68 / bridge.md's design-decision entry for -/// why a single `NoSharing`-only executor is unsound for a shared handler. +/// Sharing>*` in each — see bridge.md's design-decision entry for why a single +/// `NoSharing`-only executor is unsound for a shared handler. template inline void ActionExecuteRegistry::registerAction(std::string_view modelId, std::string_view actionId) { auto makeExecutor = []() { diff --git a/include/morph/core/callback_scope.hpp b/include/morph/core/callback_scope.hpp index 62a9380c..6042fd53 100644 --- a/include/morph/core/callback_scope.hpp +++ b/include/morph/core/callback_scope.hpp @@ -202,13 +202,12 @@ class CallbackToken { /// the others — call it from the thread that owns the scope. It replaces the /// `_state` handle itself, and reading a `shared_ptr` while another thread /// assigns it is a data race on the handle: the control block's refcount is -/// atomic, the pointer object is not. This is a narrower promise than this -/// paragraph used to make, and the reason is a portability constraint, not -/// taste — see `_state`'s own comment. In the usage `reset()` exists for +/// atomic, the pointer object is not. That narrowing is a portability +/// constraint, not taste — see `_state`'s own comment. In the usage `reset()` exists for /// (`void onNewQuery() { _callbacks.reset(); }`, the supersede verb) the owning -/// thread is the caller anyway. Within a single generation, the old guarantee -/// holds unchanged: `reset()` stops the outgoing generation before releasing -/// it, so a token holder that pinned it still observes refusal (morph#499). +/// thread is the caller anyway. Within a single generation the guarantee is +/// unconditional: `reset()` stops the outgoing generation before releasing it, +/// so a token holder that pinned it still observes refusal. /// /// Identity, not a value: neither copyable nor movable. A moved-from scope would /// have to either strand or silently retarget tokens already captured in flight; @@ -237,8 +236,7 @@ class CallbackScope { void requestStop() const noexcept { // `_state` is never null: the sole constructor make_shared's it, the // class is non-copyable and non-movable, and `reset()` always assigns a - // fresh value. The former `!= nullptr` guard was an unreachable branch - // and is gone (morph#499). + // fresh value. A `!= nullptr` guard here would be an unreachable branch. _state->stopped.store(true, std::memory_order_release); } @@ -287,7 +285,7 @@ class CallbackScope { /// the primary template and hard-errors on `is_trivially_copyable`. The /// alternative, a mutex, would cost `token()`/`stopRequested()` their /// `noexcept`. So the concurrency contract is narrowed instead; see this - /// class's own Thread safety paragraph (morph#499). + /// class's own Thread safety paragraph. std::shared_ptr _state; }; diff --git a/include/morph/core/completion.hpp b/include/morph/core/completion.hpp index 445a3fcc..7adc6672 100644 --- a/include/morph/core/completion.hpp +++ b/include/morph/core/completion.hpp @@ -60,8 +60,8 @@ struct CompletionState : std::enable_shared_from_this> { // `std::function` object -- because each is invocable with // `const T&`. A handler that wants its own value gets exactly one copy, at // its own parameter binding, where the reader of that call site can see it; - // a handler that only observes pays nothing. Erasing as `void(T)` charged - // every handler a copy whether or not it wanted one (morph#553). + // a handler that only observes pays nothing. Erasing as `void(T)` would + // charge every handler a copy whether or not it wanted one. std::vector> onOk; std::vector> onErr; bool onErrAttached = false; @@ -77,8 +77,8 @@ struct CompletionState : std::enable_shared_from_this> { // Store first, drain `onOk` last. `value` is this state's own // store and is never moved out of: a `then()` attached *after* // this point (attachThen's `ready && value` branch) reads it - // again, and moving out of it left it engaged but moved-from so a - // later attacher silently observed a husk (morph#520). The value + // again, and moving out of it would leave it engaged but + // moved-from, so a later attacher would silently observe a husk. The value // is observed, never consumed -- structurally, now that no // dispatch path can take it. // @@ -155,7 +155,7 @@ struct CompletionState : std::enable_shared_from_this> { // `exception_ptr` straight through (`.onError([state](auto e) { // state->setException(e); })`) without inspecting it, so a guard // at one producer would leave every other one able to reintroduce - // the same wedge. See issue #347. + // the same wedge. error = exc ? exc : std::make_exception_ptr( std::runtime_error{"completion rejected with no exception (null exception_ptr)"}); @@ -192,11 +192,10 @@ struct CompletionState : std::enable_shared_from_this> { std::scoped_lock const lock{mtx}; if (ready && value) { // Keep the state alive and read `value` in place rather than - // snapshotting it. The old shape copied `*value` into - // `savedVal` and then captured `savedVal` *by copy* before - // moving it into the handler -- two copies where the handler - // asked for at most one, and the reason a late attacher cost - // 2 copies rather than 1 (morph#553). + // snapshotting it. Copying `*value` into a local and then + // capturing that local *by copy* before moving it into the + // handler costs two copies where the handler asked for at most + // one, so a late attacher would pay 2 rather than 1. fireNow = [self = this->shared_from_this(), handler = std::move(handler)]() { handler(*self->value); }; } else if (!ready) { onOk.push_back(std::move(handler)); @@ -255,10 +254,10 @@ struct CompletionState : std::enable_shared_from_this> { /// completion state, instead of a second, erased completion whose only job is /// to be forwarded into the first. /// -/// That forwarding cost six heap allocations per dispatch — the erased state, +/// Such forwarding costs six heap allocations per dispatch — the erased state, /// the `.then` and `.onError` closures, their two handler vectors, and one of -/// the two posted settle tasks — of the 14.06 a local round trip took. Through -/// a sink it is one (morph#572, Part B). +/// the two posted settle tasks — against the 14.06 a local round trip takes. +/// Through a sink it is one. /// /// @par Contract for implementers /// - **Settle once.** `settleValue` and `settleException` are mutually @@ -579,7 +578,7 @@ class Completion { /// @brief Constructs a `Completion`/`Promise` pair sharing one settleable state. /// - /// The public "settleable promise" seam (issue #55): lets a caller — typically + /// The public "settleable promise" seam: lets a caller — typically /// test code — construct a `Completion` it can resolve or reject on demand, /// without a full `Bridge`/`IBackend` round trip and without reaching into /// `morph::async::detail::CompletionState`. Everything `Completion(state, diff --git a/include/morph/core/detail/execute_order_gate.hpp b/include/morph/core/detail/execute_order_gate.hpp index 952ef3b5..7fb58657 100644 --- a/include/morph/core/detail/execute_order_gate.hpp +++ b/include/morph/core/detail/execute_order_gate.hpp @@ -17,29 +17,20 @@ /// @file /// @brief Per-model execute-ordering gate, extracted out of `RemoteServer`. /// -/// `RemoteServer`'s per-model execute-ordering gate (`ExecuteGate` before this -/// extraction, `takeExecuteTicket`/`awaitExecuteTurn`/`releaseExecuteTicket`, -/// and `ExecuteTicketGuard` — all `private`) is a standalone concurrency -/// primitive that was trapped inside the server. Its contract — hand out -/// monotonic tickets per `ModelId`; block a ticket until its predecessor -/// releases; tolerate releases arriving out of order; erase the gate when -/// drained; tolerate a gate already erased — touches no wire format, no -/// authorizer, no model, no executor, no strand, and is expressible without -/// `RemoteServer` at all. +/// The per-model execute-ordering gate `RemoteServer` dispatches through. Its +/// contract — hand out monotonic tickets per `ModelId`; block a ticket until +/// its predecessor releases; tolerate releases arriving out of order; erase the +/// gate when drained; tolerate a gate already erased — touches no wire format, +/// no authorizer, no model, no executor and no strand, so it lives here rather +/// than inside the server. /// -/// It began as a behavior-preserving port of the logic that used to live -/// directly on `RemoteServer` -- `releasedOutOfOrder` and its out-of-order -/// release handling (issue #449) came across unchanged, and that field's own -/// doc comment carries the full history. -/// -/// It is **no longer only that port**, and the locking in particular is not the -/// same. morph#519 added the atomic `takeAndPost`, a nested `Ticket` bound to -/// the exact `Gate` it was issued from (so a drain-and-recreate cannot redirect -/// a later `awaitTurn`), and a gate-wide `std::recursive_mutex` held across the -/// caller's `postFn`. Read `takeAndPost`'s doc before reasoning about lock -/// order here: it is `_enqueueMtx` then `_mtx`, never the reverse, and an -/// earlier revision that used one mutex *per model* deadlocked two threads -/// against each other. +/// `takeAndPost` takes a ticket and enqueues the work in one atomic step, the +/// `Ticket` it returns is bound to the exact `Gate` it was issued from (so a +/// drain-and-recreate cannot redirect a later `awaitTurn`), and a gate-wide +/// `std::recursive_mutex` is held across the caller's `postFn`. Read +/// `takeAndPost`'s doc before reasoning about lock order here: it is +/// `_enqueueMtx` then `_mtx`, never the reverse. One mutex *per model* instead +/// deadlocks two threads against each other. namespace morph::backend::detail { /// @brief Hands out monotonic per-`ModelId` tickets and lets callers block @@ -68,10 +59,10 @@ class ExecuteOrderGate { /// ahead of the workers), can find a *newer* `Gate` already sitting where /// the old one used to be. Its `awaitTurn` then waits on that new gate's /// independent counter for a ticket number it will never produce -- - /// permanently (morph#519's own fix introduced this: the old two-step - /// `take()`-then-post() throttled the producer just enough that a gate - /// essentially never drained mid-burst; the atomic `takeAndPost` removes - /// that throttle). `Ticket` closes this by carrying the exact `Gate` + /// permanently. The atomic `takeAndPost` is what makes this reachable: a + /// two-step `take()`-then-post() throttles the producer just enough that a + /// gate essentially never drains mid-burst, and `takeAndPost` removes that + /// throttle. `Ticket` closes the hole by carrying the exact `Gate` /// `shared_ptr` a ticket was issued from, so `awaitTurn(Ticket)` and /// `release(Ticket)` operate on that object directly -- never a fresh /// lookup, so a drain-and-recreate of the map entry cannot redirect them. @@ -119,7 +110,7 @@ class ExecuteOrderGate { // inserting an already-passed ticket number into `releasedOutOfOrder` // a second time, which would sit there as the set's permanent // minimum and quietly break every future out-of-order release for - // this gate (issue #449's own mechanism, from the wrong end). + // this gate. std::shared_ptr> _released; }; @@ -181,9 +172,9 @@ class ExecuteOrderGate { /// diverge: caller A can take ticket 0 and then be pre-empted before /// enqueueing, while caller B takes ticket 1 and enqueues immediately — so /// a pool worker picks up ticket 1 first, blocks in `awaitTurn` waiting for - /// ticket 0, and A's own enqueued work never gets a worker to run on - /// (morph#519: exactly this, with `RemoteServer::handleImpl` and its worker - /// pool). Folding the enqueue into the same critical section as `take` + /// ticket 0, and A's own enqueued work never gets a worker to run on -- + /// reachable with `RemoteServer::handleImpl` and its worker pool. + /// Folding the enqueue into the same critical section as `take` /// makes that divergence impossible: whichever caller's `takeAndPost` runs /// first for a given model gets both the lower ticket number and the /// earlier enqueue slot, for any thread scheduling, because a second @@ -388,7 +379,7 @@ class ExecuteOrderGate { // ticket ahead of them has released too. Not an optimisation: it is // what makes `nextToRun` mean "the lowest ticket not yet released" // rather than "one past whichever ticket released last". See - // `releaseOnGateLocked` (issue #449). Ordered, because the release + // `releaseOnGateLocked`. Ordered, because the release // loop consumes it from the front; small by construction (it holds at // most the tickets in flight for one model, minus one). std::set releasedOutOfOrder; @@ -426,12 +417,10 @@ class ExecuteOrderGate { // immediately, without ever waiting for its turn, precisely so that // ticket cannot hold up the live work behind it (`RemoteServer`'s // rejection paths -- model not found, unauthorized, over limit, a - // shutdown gate -- all do exactly this). A later ticket releasing - // first therefore used to push `nextToRun` straight past an earlier - // ticket's number, whose waiter then had a predicate that could never - // become true again -- a caller parked forever (issue #449, the third - // occurrence of the stranded-ticket class #348 and #351 closed from - // the other end by making the release itself unmissable). + // shutdown gate -- all do exactly this). Applying a later ticket's + // release directly would push `nextToRun` straight past an earlier + // ticket's number, leaving that ticket's waiter with a predicate that + // can never become true again -- a caller parked forever. // // So an out-of-order release is recorded rather than applied, and // `nextToRun` walks forward only over a contiguous run of released @@ -504,9 +493,9 @@ class ExecuteOrderGate { /// stalls every later ticket for the same model, because `awaitTurn` is a /// `cv.wait` with no deadline. In `RemoteServer` (the sole caller today) that /// rule used to be a per-call-site convention, and the convention was missed -/// twice: by a shutdown gate that returns before the one place that released -/// a ticket (issue #348), and by every exception that unwinds past a dispatch -/// function's early returns (issue #351). This holder makes the rule +/// two ways: by a shutdown gate that returns before the one place that +/// releases a ticket, and by an exception that unwinds past a dispatch +/// function's early returns. This holder makes the rule /// structural instead of remembered: the ticket is owned from the moment it /// is taken until the guard dies, so every exit path -- `return`, `throw`, /// and any branch a later change adds -- releases it. Two members opt out diff --git a/include/morph/core/detail/instance_directory.hpp b/include/morph/core/detail/instance_directory.hpp index 12174879..9aa28675 100644 --- a/include/morph/core/detail/instance_directory.hpp +++ b/include/morph/core/detail/instance_directory.hpp @@ -39,7 +39,7 @@ enum class Hydration : std::uint8_t { /// cannot be moved between its states atomically: a reader landing between /// "first action is no longer pending" and "…and it failed" observes a /// not-poisoned instance whose first action has already failed, and is handed -/// it — the exact case the spec forbids (morph#523). +/// it — the exact case the spec forbids. /// /// Owned through a `shared_ptr` and captured that way — never via a raw pointer /// to the owning backend — into the strand task that settles it: that task may @@ -74,8 +74,8 @@ using DirectoryKey = std::pair; /// Structurally identical to `registry.hpp`'s `::morph::model::detail::PairKeyHash`, /// but defined here rather than reused so this header depends on nothing beyond /// `model.hpp`: `registry.hpp` pulls in glaze and the forms/schema stack, and the -/// instance directory is part of the async core that must stay usable without it -/// (morph#521). +/// instance directory is part of the async core that must stay usable without +/// it. struct DirectoryKeyHash { /// @brief Combines the hashes of the type id and the primary key. /// @param key The directory key to hash. @@ -91,9 +91,9 @@ struct DirectoryKeyHash { /// @brief Everything a backend knows about one live model instance. /// /// One record per instance, rather than one entry per instance in each of -/// several `ModelId`-keyed maps that had to be kept in lockstep by convention -/// (morph#523). Every field is reached through a single hash lookup, and a -/// half-updated instance is not representable. +/// several `ModelId`-keyed maps kept in lockstep by convention. Every field is +/// reached through a single hash lookup, and a half-updated instance is not +/// representable. struct Instance { /// @brief The model itself. Never null for a record that is in the directory. std::shared_ptr<::morph::model::detail::IModelHolder> holder; @@ -131,11 +131,11 @@ struct Instance { /// @brief The live model instances of one backend, plus the shared-instance directory. /// -/// Replaces the six (`LocalBackend`) and eight (`RemoteServer`) parallel -/// `ModelId`-keyed containers those classes used to carry, and the -/// register-or-attach logic that was written out twice (morph#523). The -/// invariants it maintains are stated in `docs/spec/core/shared_instances.md`; -/// this class is where they are now enforced rather than asserted in comments. +/// One type, shared by `LocalBackend` and `RemoteServer`, in place of the six +/// and eight parallel `ModelId`-keyed containers each would otherwise carry and +/// two separate copies of the register-or-attach logic. The invariants it +/// maintains are stated in `docs/spec/core/shared_instances.md`; this class is +/// where they are enforced rather than asserted in comments. /// /// @par Locking /// **Caller-locked, deliberately.** Every operation below assumes the owning diff --git a/include/morph/core/detail/reply_router.hpp b/include/morph/core/detail/reply_router.hpp index 16bb3936..ce8dacf4 100644 --- a/include/morph/core/detail/reply_router.hpp +++ b/include/morph/core/detail/reply_router.hpp @@ -37,7 +37,7 @@ /// net/` belongs to the optional `morph_net` target (`MORPH_BUILD_NET`, /// default `OFF`). A `core/` header including a `net/` one would therefore /// install a `remote.hpp` that cannot find its own include whenever net is -/// off — the same breakage morph#232 fixed for `quantity.hpp`. Living under +/// off. Living under /// `core/detail/` keeps the dependency pointing the one direction it /// already points (`net` → `core`). diff --git a/include/morph/core/executor.hpp b/include/morph/core/executor.hpp index b324efe1..be600b11 100644 --- a/include/morph/core/executor.hpp +++ b/include/morph/core/executor.hpp @@ -219,7 +219,6 @@ class MainThreadExecutor : public IExecutor { // drain(), each of which documents that a throwing task is logged // and the pump continues -- runOnce() promises to return `true` // "whether or not that task threw", and would not return at all. - // morph#501. ::morph::log::logError("[main-thread] callback threw unknown exception"); } } diff --git a/include/morph/core/file_io_ops.hpp b/include/morph/core/file_io_ops.hpp index 057ef2be..cb18fc9a 100644 --- a/include/morph/core/file_io_ops.hpp +++ b/include/morph/core/file_io_ops.hpp @@ -58,8 +58,8 @@ int retryOnEintr(Operation operation) { /// file-I/O call fails partway through an otherwise-successful operation /// (disk full, fd closed underneath, a permission change racing an exact /// window). None of those are reachable from a portable unit test without -/// this seam — see `LASTRADA-Software/morph#97`, which requested exactly -/// this for `FileActionLog`; `FileOfflineQueue` has the identical gap. A +/// this seam, and `FileActionLog` and `FileOfflineQueue` have the identical +/// gap. A /// test constructs a `FileIoOps` whose relevant member fails on demand (or /// on the Nth call, or forever) and passes it to the class under test; /// every other member stays at its real default, so the rest of the class's @@ -128,7 +128,7 @@ struct FileIoOps { /// it) to durable storage. `fsync` on a *file* makes only that /// file's data durable -- not the directory entry that names it, /// so a fresh file's creation or a rename can vanish on power loss - /// even after the file's own contents were fsynced (morph#532). + /// even after the file's own contents were fsynced. /// POSIX: `open(dir, O_RDONLY|O_DIRECTORY)` + `fsync` + `close`. A /// no-op on Windows, documented as such rather than faked -- /// `FlushFileBuffers`'s semantics for a directory handle differ @@ -261,8 +261,8 @@ enum class RollBack : std::uint8_t { /// it, `std::ftell` returns `-1` (`EOVERFLOW`) on every call, which /// `rollBackShortWrite()` already treats as "no offset to roll back /// to" and silently skips — quietly reviving the exact torn-tail -/// merge morph#530 fixed, on every platform where this actually -/// matters, for as long as the process keeps running (the next +/// merge the rollback exists to prevent, on every platform where this +/// actually matters, for as long as the process keeps running (the next /// restart's `repairTornTail()` is still a backstop, but a long-lived /// process that never restarts gets no benefit from it). /// @param file Open stdio handle to query. @@ -308,7 +308,7 @@ inline void positionAtEnd(std::FILE* file) noexcept { } /// @brief Rolls @p file/@p path back to @p offsetBeforeWrite bytes after a -/// short write, best-effort (morph#530). +/// short write, best-effort. /// /// `resizeFile` truncates the file by path, not through @p file's own file /// descriptor, so @p file's buffered stdio position (what a later `ftell` @@ -326,8 +326,8 @@ inline void positionAtEnd(std::FILE* file) noexcept { /// `std::filesystem::resize_file` **grows** it, padding with NUL bytes, and any /// later flush then appends the buffered record *after* that padding. The /// result is a NUL-bearing interior line that the caller's own reader rejects -/// for the life of the file: precisely the bricking morph#530 exists to -/// prevent, manufactured by the rollback meant to prevent it. (Measured: +/// for the life of the file: precisely the bricking this rollback exists to +/// prevent, manufactured by the rollback itself. (Measured: /// `ftell`=30 against an on-disk size of 10, `resize_file(30)` yielding a /// 30-byte file, and a final 50-byte file of data + 20 NULs + the flushed /// record.) @@ -357,7 +357,7 @@ inline void positionAtEnd(std::FILE* file) noexcept { /// position where neither heal applies. Measured, against a queue: the merged /// line makes the next open throw a raw parse error instead of loading, so the /// whole backlog -- including records written long before the failure -- becomes -/// unreachable. That is the bricking morph#530 exists to prevent, reached +/// unreachable. That is the bricking the rollback exists to prevent, reached /// through the rollback rather than around it. A caller that gets `torn` must /// therefore refuse further appends on this handle rather than carry on. /// @@ -404,8 +404,8 @@ inline void positionAtEnd(std::FILE* file) noexcept { /// @brief Truncates any bytes following the last newline in @p path. /// -/// A crash between a caller's `fwrite` and its next `fsync` (or a short -/// write, before morph#530's fix) can leave a partial record at the end. +/// A crash between a caller's `fwrite` and its next `fsync` can leave a +/// partial record at the end. /// Because every complete record is written newline-terminated in a single /// `fwrite`, whatever follows the final newline is by construction an /// incomplete record and never a whole one -- which makes discarding it diff --git a/include/morph/core/registry.hpp b/include/morph/core/registry.hpp index e89c7545..29502818 100644 --- a/include/morph/core/registry.hpp +++ b/include/morph/core/registry.hpp @@ -49,7 +49,7 @@ struct ActionTraits; // during static initialisation, and after `main()` begins the maps are // read-only. Nothing detected a violation. A `dlopen`ed module registering on // a worker thread while another thread dispatches is undefined behaviour whose -// only symptom is intermittent map corruption (morph#698). +// only symptom is intermittent map corruption. // // This latch makes that precondition checkable. It starts open, closes when // the program says the registration phase is over, and the three `register*Once` @@ -143,9 +143,9 @@ inline void noteRegistryRead([[maybe_unused]] bool isProcessRegistry) noexcept { /// pair of `std::string`s, because the registry owns its ids; a caller has /// `string_view`s, because every id it can name arrives as one — a wire /// envelope's decoded field, or a `constexpr` `ModelTraits::typeId()`. -/// Materialising the stored type just to hash it charged every lookup up to -/// two `std::string` constructions (morph#572, Part C), so the hash and -/// equality below are transparent and this is what `find()` is given. +/// Materialising the stored type just to hash it would charge every lookup up +/// to two `std::string` constructions, so the hash and equality below are +/// transparent and this is what `find()` is given. using PairKeyView = std::pair; /// @brief Transparent hash functor for `(modelId, actionId)` registry keys. @@ -710,13 +710,13 @@ class ActionDispatcher { std::string dispatch(std::string_view modelId, std::string_view actionId, IModelHolder& holder, std::string_view payload) { // A dispatch means the maps are being read, which in the registration - // model means the registration phase is over (morph#698). Debug builds - // only; see `detail::noteRegistryRead`. + // model means the registration phase is over. Debug builds only; see + // `detail::noteRegistryRead`. detail::noteRegistryRead(this == &defaultDispatcher()); - // Looked up by view. Building the stored `Key` to hash it cost two - // `std::string` constructions per dispatch -- and two heap allocations - // whenever an id passed the SSO threshold, which `"CreateSwimlane"` - // (14 characters) misses by one. See morph#572, Part C. + // Looked up by view. Building the stored `Key` to hash it would cost + // two `std::string` constructions per dispatch -- and two heap + // allocations whenever an id passes the SSO threshold, which + // `"CreateSwimlane"` (14 characters) misses by one. auto iter = _actions.find(detail::PairKeyView{modelId, actionId}); if (iter == _actions.end()) { // The concatenation here is the one string cost left, and it is on @@ -847,13 +847,13 @@ class ActionDispatcher { /// @brief Everything registered under one `(modelId, actionId)` pair. /// - /// One record, not four parallel maps keyed identically. The four were - /// filled together by `registerAction` and could not go out of step in - /// practice, but nothing said so -- and `RemoteServer::handle` pays for - /// three separate lookups of the same key on the way through one request - /// (`schemaFor`, `requiredFieldsFor`, `dispatch`). Merging them makes the - /// lockstep structural and makes those three one hash each rather than one - /// hash into a different table each (morph#572, Part C). + /// One record, not four parallel maps keyed identically. Four maps filled + /// together by `registerAction` could not go out of step in practice, but + /// nothing would say so -- and `RemoteServer::handle` reaches this key + /// three times on the way through one request (`schemaFor`, + /// `requiredFieldsFor`, `dispatch`). One record makes the lockstep + /// structural and makes those three one hash each rather than one hash + /// into a different table each. struct ActionEntry { /// @brief Type-erased decode/execute/encode runner. Runner runner; @@ -924,25 +924,24 @@ class ModelRegistryFactory { void registerModel(std::string_view modelId, Factory factory) { _factories.insert_or_assign(std::string{modelId}, [factory = std::move(factory)]() mutable { std::unique_ptr holder{factory()}; - // The fourth site of the morph#742 defect, and the one nothing was guarding. - // misc-static-assert fires here too -- `holder` is a runtime + // misc-static-assert fires here -- `holder` is a runtime // `std::unique_ptr` the factory just returned, so nothing in this condition // is a constant expression, and the check's own fix does not compile: // "static assertion expression is not an integral constant expression / // function parameter 'holder' with unknown value cannot be used in a constant - // expression". Same defect, same suppression, same upstream report (morph#742). + // expression". Same finding and same suppression as the three + // `register*Once` helpers below. // - // It stayed green only because `clang-tidy-diff` analyses changed lines and - // nothing had changed this one, so the next person to edit it inherited a red - // build that was not theirs (morph#762). + // Note the suppression is needed even though `clang-tidy-diff` analyses + // changed lines only: without it, the next person to edit this line + // inherits a red build that is not theirs. // // Last re-checked at clang-tidy 22.1.8, the version CI pins: the diagnostic // still fires and the suggested fix still does not compile. The stamp is here - // because nothing checks it for you -- morph#755 deleted both - // scripts/check_nolint_directives.sh (which would have flagged a directive that - // had stopped suppressing anything) and scripts/check_ci_clang_pin.sh (the - // natural re-check trigger on a CLANG_VERSION bump). Re-read this when the pin - // moves; if the finding is gone, delete all four directives together. + // because nothing re-checks it for you -- no script flags a directive that has + // stopped suppressing anything, and nothing triggers a re-read on a + // CLANG_VERSION bump. Re-read this when the pin moves; if the finding is gone, + // delete all four directives together. // NOLINTNEXTLINE(misc-static-assert,cert-dcl03-c) assert(!holder || (holder->type() == std::type_index(typeid(Model)) && @@ -997,7 +996,7 @@ inline ModelRegistryFactory& defaultRegistry() { } // The three `register*Once` helpers are `noexcept` while allocating, and that -// is a decision rather than an oversight (morph#698). Their only caller is the +// is a decision rather than an oversight. Their only caller is the // initialiser of a namespace-scope variable generated by `BRIDGE_REGISTER_*`, // and [basic.start.dynamic]/[except.terminate] already call `std::terminate` // when an exception escapes the dynamic initialisation of a non-local @@ -1015,20 +1014,19 @@ inline bool registerModelOnce(std::string_view modelId) noexcept { // Applying the check's own fix does not compile: `static_assert` on this // condition is "static assertion expression is not an integral constant // expression / non-constexpr function 'registrationPhaseClosed' cannot be used - // in a constant expression". Reported upstream of this repository in morph#742. + // in a constant expression". The finding is upstream of this repository. // // Last re-checked at clang-tidy 22.1.8, the version CI pins: the diagnostic // still fires and the suggested fix still does not compile. The stamp is here - // because nothing checks it for you -- morph#755 deleted both - // scripts/check_nolint_directives.sh (which would have flagged a directive that - // had stopped suppressing anything) and scripts/check_ci_clang_pin.sh (the - // natural re-check trigger on a CLANG_VERSION bump). Re-read this when the pin - // moves; if the finding is gone, delete all four directives together. + // because nothing re-checks it for you -- no script flags a directive that has + // stopped suppressing anything, and nothing triggers a re-read on a + // CLANG_VERSION bump. Re-read this when the pin moves; if the finding is gone, + // delete all four directives together. // NOLINTNEXTLINE(misc-static-assert,cert-dcl03-c) assert(!::morph::model::registrationPhaseClosed() && "registerModelOnce: registration after the registration phase closed. The process-level " "registries are unsynchronised and are read-only once dispatch begins -- registering now " - "races their internals against concurrent lookups (morph#698; docs/spec/core/registry.md, " + "races their internals against concurrent lookups (see docs/spec/core/registry.md, " "\"Thread safety\"). Load and register plugin modules before the first dispatch."); ModelRegistryFactory::instance().registerModel(modelId); return true; @@ -1042,20 +1040,19 @@ inline bool registerActionOnce(std::string_view modelId, std::string_view action // Applying the check's own fix does not compile: `static_assert` on this // condition is "static assertion expression is not an integral constant // expression / non-constexpr function 'registrationPhaseClosed' cannot be used - // in a constant expression". Reported upstream of this repository in morph#742. + // in a constant expression". The finding is upstream of this repository. // // Last re-checked at clang-tidy 22.1.8, the version CI pins: the diagnostic // still fires and the suggested fix still does not compile. The stamp is here - // because nothing checks it for you -- morph#755 deleted both - // scripts/check_nolint_directives.sh (which would have flagged a directive that - // had stopped suppressing anything) and scripts/check_ci_clang_pin.sh (the - // natural re-check trigger on a CLANG_VERSION bump). Re-read this when the pin - // moves; if the finding is gone, delete all four directives together. + // because nothing re-checks it for you -- no script flags a directive that has + // stopped suppressing anything, and nothing triggers a re-read on a + // CLANG_VERSION bump. Re-read this when the pin moves; if the finding is gone, + // delete all four directives together. // NOLINTNEXTLINE(misc-static-assert,cert-dcl03-c) assert(!::morph::model::registrationPhaseClosed() && "registerActionOnce: registration after the registration phase closed. The process-level " "registries are unsynchronised and are read-only once dispatch begins -- registering now " - "races their internals against concurrent lookups (morph#698; docs/spec/core/registry.md, " + "races their internals against concurrent lookups (see docs/spec/core/registry.md, " "\"Thread safety\"). Load and register plugin modules before the first dispatch."); ActionDispatcher::instance().registerAction(modelId, actionId); return true; diff --git a/include/morph/core/remote.hpp b/include/morph/core/remote.hpp index 4c686a70..249f23fb 100644 --- a/include/morph/core/remote.hpp +++ b/include/morph/core/remote.hpp @@ -343,10 +343,10 @@ class RemoteServer : public std::enable_shared_from_this { /// same-model `execute`s posted back-to-back always reach the pool's queue /// in ticket order — the same order the transport called `handle()` in, /// i.e. send order — no matter how the calling threads are scheduled - /// relative to each other (morph#519: taking the ticket and enqueueing as - /// two separate, unlocked steps let two concurrent transport threads' - /// tickets and enqueue order diverge, which could park every pool worker - /// in `awaitTurn` permanently). If this peek fails to decode at all, or + /// relative to each other. Taking the ticket and enqueueing as two + /// separate, unlocked steps would let two concurrent transport threads' + /// tickets and enqueue order diverge, which can park every pool worker in + /// `awaitTurn` permanently. If this peek fails to decode at all, or /// isn't an `execute`, no ticket is taken; `dispatchMessage` still does the /// real (only) decode moments later on the pool thread and produces the /// canonical error for genuinely malformed input — this peek only ever @@ -791,8 +791,7 @@ class RemoteServer : public std::enable_shared_from_this { // releasing it here cancels out only the redundant reference // that call just took, never the caller's sole hold on the // instance it is "re-pointing" to itself. A genuinely - // different-key re-point releases the real old instance, same - // as before. + // different-key re-point releases the real old instance. if (releaseCurrent.v != 0U) { releaseScopedLocked(releaseCurrent, cid); } @@ -877,7 +876,7 @@ class RemoteServer : public std::enable_shared_from_this { /// judged on the **key**, not on the decoded value, because that is the /// only question the action codec cannot answer: `fromJson` turns an /// absent field and an explicitly-sent zero into the same action, which is - /// exactly why a renamed field survives `validate()` today (morph#207). + /// exactly why a renamed field survives `validate()`. /// /// Conservative in both directions. A `body` that is not a JSON object at /// all reports nothing missing — malformed JSON is `fromJson`'s error to @@ -932,9 +931,8 @@ class RemoteServer : public std::enable_shared_from_this { /// Extracted, verbatim in behavior, to /// `morph::backend::detail::ExecuteTicketGuard` /// (`include/morph/core/detail/execute_order_gate.hpp`) — see that - /// class's own doc comment for the full design rationale (issues #348, - /// #351, #449). Aliased here so every existing call site in this class - /// keeps reading `ExecuteTicketGuard` unqualified. + /// class's own doc comment for the full design rationale. Aliased here so + /// every call site in this class reads `ExecuteTicketGuard` unqualified. using ExecuteTicketGuard = ::morph::backend::detail::ExecuteTicketGuard; // One flat switch over the wire's `kind` discriminator. Splitting it would @@ -955,7 +953,7 @@ class RemoteServer : public std::enable_shared_from_this { // the outer catch, and any branch a later change adds — releases the // ticket, which is what makes `ExecuteOrderGate::release`'s stated // rule structurally true rather than a convention each call site has - // to remember (issues #348 and #351). + // to remember. ExecuteTicketGuard ticketGuard{_executeGate, std::move(executeTicket)}; ::morph::wire::Envelope env; try { @@ -1003,9 +1001,8 @@ class RemoteServer : public std::enable_shared_from_this { // nothing about it is worth making a later same-model `execute` // wait for. // - // Not merely a leaked map entry on a dying server (which is what - // this branch was previously believed to cost): tickets are taken - // in send order on the transport thread, but the pool is free to + // Not merely a leaked map entry on a dying server: tickets are + // taken in send order on the transport thread, but the pool is free to // run the two posted tasks in either order. A *later* ticket that // passed this gate before `beginShutdown()` is already parked in // `ExecuteOrderGate::awaitTurn`, on a `cv.wait` with no deadline, @@ -1260,9 +1257,9 @@ class RemoteServer : public std::enable_shared_from_this { // `missingRequiredFields` steps are all reachable, non-`noexcept` // code — unwinds past every explicit release to here. `ticketGuard` // is what releases the ticket on this path; it is destroyed as this - // frame returns. Before it existed, this catch replied and stranded - // the ticket, and the next same-model `execute` waited on it - // forever (issue #351). + // frame returns. Replying here without releasing would strand the + // ticket, and the next same-model `execute` would wait on it + // forever. reply(::morph::wire::encode(::morph::wire::makeErr(exc.what(), env.callId))); } } @@ -1283,7 +1280,7 @@ class RemoteServer : public std::enable_shared_from_this { // `authorize`/`authenticate`/`authorizeInstance`/`missingRequiredFields`, // or out of the post itself — is covered by the guard's destructor back in // `dispatchMessage`. See `ExecuteTicketGuard` and the class-private - // members' own doc comment for the full design (issue #351). + // members' own doc comment for the full design. // NOLINTNEXTLINE(readability-function-cognitive-complexity) void dispatchExecute(::morph::wire::Envelope env, std::function reply, ExecuteTicketGuard& ticketGuard) { @@ -1400,7 +1397,7 @@ class RemoteServer : public std::enable_shared_from_this { // can claim the same completion slot `complete` uses. Everything between // the increment and the strand post is non-`noexcept` -- emitMetric, two // make_shared, TimeoutScheduler::schedule, awaitTurn's mutex, the post - // itself -- and a throw there used to leak the slot permanently. + // itself -- and a throw there would otherwise leak the slot permanently. auto finished = std::make_shared(); std::size_t inFlightAfterInc = 0; @@ -1435,7 +1432,7 @@ class RemoteServer : public std::enable_shared_from_this { // Without this, one throw left `_inFlightExecutes` permanently // over-counted: `drainedWithin()` predicates on it reaching zero, so // graceful shutdown could never succeed again for that server, and with - // `maxInFlightExecutes` set a slot was lost for good (morph#502). + // `maxInFlightExecutes` set a slot would be lost for good. struct InFlightReservation { RemoteServer* server; std::shared_ptr finished; @@ -1510,7 +1507,7 @@ class RemoteServer : public std::enable_shared_from_this { // (see that function's comment). A timeout callback already // mid-flight when the dispatch finishes therefore still calls // `complete`, and `complete`'s reply-exactly-once flag drops - // it rather than double-answering the call (morph#620). + // it rather than double-answering the call. timeoutHandle = _timeoutScheduler->schedule(limits.executeTimeout, [complete, callId]() mutable { complete(::morph::wire::encode(::morph::wire::makeErr("timeout", callId))); }); @@ -1533,8 +1530,8 @@ class RemoteServer : public std::enable_shared_from_this { // rejected request was sent after this one, which is why // `ExecuteOrderGate::release` advances `nextToRun` over a contiguous // run of released tickets rather than jumping to `ticket + 1`: jumping - // was what let a rejection skip past this wait's ticket and park it - // here for good (issue #449). + // would let a rejection skip past this wait's ticket and park it here + // for good. ticketGuard.awaitTurn(); _strand.post(mid, [self, env = std::move(env), holder = std::move(holder), hydration = std::move(hydration), complete, timeoutHandle]() mutable { @@ -1671,9 +1668,9 @@ class RemoteServer : public std::enable_shared_from_this { // `handleImpl` (called directly from `handle()`, which runs on // whatever single thread the transport calls it from -- in true send // order, nothing async yet) for every `execute` with a known `modelId` - // (see `ExecuteOrderGate::takeAndPost`, morph#519 -- taking the ticket - // and posting to `_pool` as two separate, unlocked steps let two - // concurrent transport threads' ticket order and enqueue order diverge). + // (see `ExecuteOrderGate::takeAndPost` -- taking the ticket and posting + // to `_pool` as two separate, unlocked steps would let two concurrent + // transport threads' ticket order and enqueue order diverge). // `dispatchExecute` waits for its ticket's // turn only immediately before the pre-existing `_strand.post(mid, ...)` // call, and releases the next ticket's turn either right after posting @@ -1704,9 +1701,8 @@ class RemoteServer : public std::enable_shared_from_this { // lookup finding a newer generation can never redirect it. // // The gate itself -- `take`/`takeAndPost`/`awaitTurn`/`release`, the - // out-of-order-release handling that closed issue #449, the atomic - // take-and-enqueue step that closed issue #519, and the "gate already - // gone" defensive branches for #348/#351 -- is extracted to + // out-of-order-release handling, the atomic take-and-enqueue step, and + // the "gate already gone" defensive branches -- is extracted to // `morph::backend::detail::ExecuteOrderGate` // (`include/morph/core/detail/execute_order_gate.hpp`), which has its own // direct unit tests (`tests/test_execute_order_gate.cpp`). What stays here @@ -1722,8 +1718,8 @@ class RemoteServer : public std::enable_shared_from_this { // Every live instance, private and shared alike, plus the shared-instance // directory over them — holder, owner principal, attach count, directory key // and hydration state as one record per instance rather than seven parallel - // ModelId-keyed containers held in lockstep by convention (morph#523), and - // the same type LocalBackend owns rather than a second implementation of it. + // ModelId-keyed containers held in lockstep by convention, and the same + // type LocalBackend owns rather than a second implementation of it. // Guarded by `_regMtx`; `InstanceDirectory` is caller-locked by design, // because the admission checks and connection-scope updates in the same // critical sections must not be able to straddle a directory change. @@ -2129,10 +2125,9 @@ class SimulatedRemoteBackend : public detail::IBackend { } RemoteServer& _server; - // 0 = unscoped (the default constructor's behavior, unchanged); non-zero - // when constructed with a ConnectionId from server.openConnection() (see - // issue #48). Threaded through every handle()/handleInline() call this - // backend makes. + // 0 = unscoped (the default constructor's behavior); non-zero when + // constructed with a ConnectionId from server.openConnection(). Threaded + // through every handle()/handleInline() call this backend makes. ConnectionId _cid{0}; std::mutex _pendingMtx; std::vector>>> _pending; diff --git a/include/morph/core/strand.hpp b/include/morph/core/strand.hpp index a658c734..c069352f 100644 --- a/include/morph/core/strand.hpp +++ b/include/morph/core/strand.hpp @@ -143,8 +143,8 @@ class StrandExecutor { /// dispatches one action at a time against a model builds a fresh queue on /// every call and puts exactly one task in it. libstdc++'s `std::deque` /// allocates its node map *and* a first 512-byte buffer in its default - /// constructor, so that came to 576 bytes of the 760 the strand cost per - /// local dispatch (morph#660). Holding the head task in the strand makes + /// constructor, which comes to 576 bytes of the 760 a deque-only strand + /// costs per local dispatch. Holding the head task in the strand makes /// that case allocation-free; the overflow deque is constructed only when /// a second task is genuinely queued behind a running one, after which the /// cost is the deque's as before. @@ -215,9 +215,9 @@ class StrandExecutor { /// This is a pure allocation optimisation and changes no lifetime or /// locking rule. The drain step in `scheduleNext` removes the whole map /// entry as soon as the queue empties, so a workload that dispatches one - /// action at a time against a model paid for a fresh map node *and* a - /// fresh `make_shared` on every call — the 2 allocations / 152 - /// bytes that were left after morph#660 took the container's share. + /// action at a time against a model would otherwise pay for a fresh map + /// node *and* a fresh `make_shared` on every call — 2 allocations + /// and 152 bytes on top of the queue's own share. /// Rather than keep the slot alive across the drain (which would need a /// deregistration hook and would trade this churn for a per-model entry /// nothing reclaims), the drain `extract`s the node instead of erasing it diff --git a/include/morph/core/wire.hpp b/include/morph/core/wire.hpp index ac6172a2..c403234b 100644 --- a/include/morph/core/wire.hpp +++ b/include/morph/core/wire.hpp @@ -354,9 +354,9 @@ struct EscapingWriteOpts : glz::opts { /// /// `Envelope` is a union-of-all-kinds struct: an `ok` reply uses three of its /// thirteen members and a `deregister` request uses two, but glaze writes every -/// member of a struct it is handed, so a minimal reply carrying an 8-byte -/// payload cost 255 bytes on the wire, 213 of them fields the kind does not use -/// (morph#524). +/// member of a struct it is handed, so without this a minimal reply carrying an +/// 8-byte payload costs 255 bytes on the wire, 213 of them fields the kind does +/// not use. /// /// Omitting a member that holds its default is **not** a protocol change. The /// decoder default-initialises the `Envelope` and reads with @@ -388,9 +388,9 @@ struct EscapingWriteOpts : glz::opts { /// `SocketBackend::dispatchIncomingEnvelope`). A peer therefore cannot treat /// an absent `callId` as "no information"; it has to *reconstruct* the /// sentinel before it can route the frame at all. morph's own `decode` does -/// that for free by default-initialising, which is why omitting it -/// round-tripped cleanly in C++ and still broke the scenario driver, a second -/// decoder that read absence as `None` (morph#524). Emitting correlation +/// that for free by default-initialising, so omitting it round-trips cleanly +/// in C++ and still breaks the scenario driver, a second decoder that reads +/// absence as `None`. Emitting correlation /// fields unconditionally costs eleven bytes on the one message shape that /// carries a zero id, and keeps "how do I route this frame" answerable from /// the frame. @@ -609,11 +609,11 @@ inline Envelope makeHello(std::uint32_t protocolVersion = kProtocolVersion) { /// flavours of invalid UTF-8, an embedded NUL, a raw control byte and an 8 MiB /// payload all encode successfully. /// -/// That left a permanently uncovered branch guarding a real invariant. Rather -/// than reshape `Envelope` to make a test possible, or wait on a -/// fault-injection hook in glaze (LASTRADA-Software/morph#96 asked for one), -/// this puts the seam on morph's side of the boundary — where the repository -/// already puts it for the identical problem with file I/O. +/// That leaves a branch guarding a real invariant with no way to reach it. +/// Rather than reshape `Envelope` to make a test possible, or wait on a +/// fault-injection hook glaze does not offer, this puts the seam on morph's +/// side of the boundary — where the repository already puts it for the +/// identical problem with file I/O. struct WireCodecOps { /// @brief Serialises @p env into @p out. Mirrors the `glz::write` call /// `encode` would otherwise make directly. From 5d14d8cf7dca7bd601f57695f8723285a9d8f976 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 23 Sep 2026 18:35:28 +0200 Subject: [PATCH 4/5] comments(forms+net+offline+util+detail): the same pass over the rest of the library Same test per sentence as the two commits before it. The blocks that needed the most care here were the ones whose history *was* the argument: - `forms.hpp`'s nested-aggregate note enumerated two abandoned designs by ticket. The comparison is the reason the current design is right, so it stays -- as a statement about what an ancestor type list and a depth counter each cost, not about what was tried when. - `file_offline_queue.hpp`'s constructor explained a `repairTornTail()` call that is deliberately absent. Rewritten as the three current reasons it would be wrong to add one, which is what a reader needs. - `replay_ledger.hpp`'s profile of two `std::string` constructions keeps every measured figure and the condition that reopens it ("the moment a per-request caller of this class appears"). Only the citations and the revision the numbers were taken on went. `include/morph/util/rational.hpp` is untouched: a fork PR holds it. Gates on this tree: clang-format 22.1.8 clean; 1586/1586 ctest; Doxygen `--target doc` exit 0 with `WARN_AS_ERROR = FAIL_ON_WARNINGS` and `WARN_IF_UNDOCUMENTED = YES` both confirmed set in the generated Doxyfile, and zero warnings in the log. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- include/morph/core/async.hpp | 4 +- include/morph/detail/quantity_equation.hpp | 18 ++--- include/morph/forms/choice.hpp | 4 +- include/morph/forms/detail/schema_name.hpp | 11 ++- include/morph/forms/flows.hpp | 4 +- include/morph/forms/forms.hpp | 73 +++++++++---------- include/morph/forms/instance_constraints.hpp | 2 +- include/morph/forms/views.hpp | 16 ++-- include/morph/forms/widget_hints.hpp | 6 +- include/morph/net/detail/tcp_socket.hpp | 59 +++++++-------- include/morph/net/socket_backend.hpp | 53 +++++++------- include/morph/net/socket_server.hpp | 34 ++++----- include/morph/offline/file_offline_queue.hpp | 38 +++++----- include/morph/offline/replay_ledger.hpp | 29 ++++---- .../morph/offline/sqlite_offline_queue.hpp | 23 +++--- include/morph/util/quantity.hpp | 18 ++--- 16 files changed, 188 insertions(+), 204 deletions(-) diff --git a/include/morph/core/async.hpp b/include/morph/core/async.hpp index a6d9ff39..6c01488f 100644 --- a/include/morph/core/async.hpp +++ b/include/morph/core/async.hpp @@ -10,8 +10,8 @@ /// schema, and they are the part of morph that costs almost nothing to compile. /// Nothing here reaches glaze. /// -/// Measured on `master` @ c6f6d953, clang 22.1.8, `-O2 -fsyntax-only`, one -/// translation unit per header, best of three: +/// Measured with clang 22.1.8, `-O2 -fsyntax-only`, one translation unit per +/// header, best of three: /// /// | header | CPU s | preprocessed lines | /// |---|---|---| diff --git a/include/morph/detail/quantity_equation.hpp b/include/morph/detail/quantity_equation.hpp index 11b57d7a..3cad511b 100644 --- a/include/morph/detail/quantity_equation.hpp +++ b/include/morph/detail/quantity_equation.hpp @@ -166,7 +166,7 @@ struct EquationRenderer { /// /// Iterative, over an explicit worklist, for the reason spelled out on /// `render` below: the derivation of a running total is a linear chain and - /// a recursive walk of it overflows the stack (morph#574). Each node is + /// a recursive walk of it overflows the stack. Each node is /// counted exactly once (the `seen` set), so the worklist order does not /// affect the counts. /// @param root The node to start from (may be null). @@ -232,9 +232,9 @@ struct EquationRenderer { } // A node reachable by several displayed paths is settled by its // first visit; re-walking it would assign nothing new and, on a - // DAG, costs one walk per path rather than per node (morph#602: - // 31 nodes built by repeated `q = q + q` have 2^30 paths and took - // 10.3 s to render 33 short lines). + // DAG, costs one walk per path rather than per node. Measured: 31 + // nodes built by repeated `q = q + q` have 2^30 paths and take + // 10.3 s to render 33 short lines without this test. if (!settled.insert(node).second) { continue; } @@ -268,9 +268,9 @@ struct EquationRenderer { /// Takes its left operand **by value** and appends to it rather than /// concatenating both sides into a fresh string. The left operand of a /// left-leaning chain — the shape `total = total + row` records — is the - /// whole expression rendered so far, so copying it once per level made - /// rendering quadratic in the depth (morph#582: 27.7 s and a - /// 350,001-character line at 70,000 steps). Appending makes that shape + /// whole expression rendered so far, so copying it once per level makes + /// rendering quadratic in the depth — measured at 27.7 s for a + /// 350,001-character line at 70,000 steps. Appending makes that shape /// linear, amortised. A **right**-leaning chain (`a + (b + (c + …))`) is /// still quadratic — the big operand is on the copied side — and so is a /// chain of unary negations, which has to prepend; `equation()`'s step @@ -353,8 +353,8 @@ struct EquationRenderer { /// The walk is an explicit stack rather than recursion because the /// derivation of a running total (`total = total + x` in a loop) is a /// linear chain one node deep per iteration, and a recursive walk of it - /// runs the stack out. Measured on the recursive code (morph#574, 8 MiB - /// stack): `equation()` returned at 24,000 nodes and segfaulted inside + /// runs the stack out. Measured on a recursive walk with an 8 MiB stack: + /// `equation()` returned at 24,000 nodes and segfaulted inside /// `renderSymbolic` at 25,000 under clang `-O0`, returned at 50,000 and /// segfaulted at 60,000 under clang `-O2`, and segfaulted already at /// 40,000 under gcc `-O2`. Optimisation only moved the limit: unlike the diff --git a/include/morph/forms/choice.hpp b/include/morph/forms/choice.hpp index faa367b5..685f87cc 100644 --- a/include/morph/forms/choice.hpp +++ b/include/morph/forms/choice.hpp @@ -159,8 +159,8 @@ inline constexpr bool isChoice = detail::IsChoice>::value /// /// `name` is composed per instantiation rather than being the literal /// `"Choice"`: glaze keys `$defs` by it and fills each entry only once, so one -/// shared name made the second `Choice` in an action `$ref` the first one's -/// definition and be described with the wrong payload type (morph#543). See +/// shared name would make the second `Choice` in an action `$ref` the first +/// one's definition and be described with the wrong payload type. See /// `forms/detail/schema_name.hpp` for how the key is built and why it is not /// derived from `glz::name_v`. template ]; if (!def.type) { … }`. A /// `glz::meta::name` that is the same string for two instantiations therefore /// makes the second one silently `$ref` the first one's definition, and a -/// renderer that resolves the `$ref` reads the wrong payload type (morph#543). +/// renderer that resolves the `$ref` reads the wrong payload type. /// /// This header is the single place the forms layer composes those keys, so the /// rule that keeps them apart is stated once rather than once per wrapper. @@ -79,8 +79,7 @@ template /// the join injective: without that escape two different splits of the same /// characters alias (`ValueField = "id_x", LabelField = "name"` against /// `"id", "x_name"` — both plausible snake_case wire names — would produce one -/// key, and the second `Choice` would then `$ref` the first one's definition, -/// which is the very defect morph#543 is about). +/// key, and the second `Choice` would then `$ref` the first one's definition). /// /// Doubling makes every `_` run *inside* an escaped part even-length, so a run /// that contains a join is odd — which is what tells the two apart, and it @@ -227,8 +226,8 @@ struct ShapeTag { /// `{"type":"string"}` for it while `std::int8_t` (i.e. `signed char`) gets /// `{"type":"integer", …}`. Both are arithmetic and both are 8 bits wide, so a /// tag built from signedness and width alone would put them on one `$defs` -/// entry describing only one of them — morph#543 again. The sibling character -/// types are listed with it because they are schematised the same way. +/// entry describing only one of them. The sibling character types are listed +/// with it because they are schematised the same way. /// /// @tparam T Type to test. template @@ -293,7 +292,7 @@ inline constexpr auto rangedSchemaNameStorage = [] { /// themselves are emitted as property-level `x-min`/`x-max`/`x-step`, never /// into the `$def`. So `Ranged<0, 100>` and `Ranged<5, 50>` share one entry /// because their entries *are* the same entry, while `Ranged<0.0, 1.0, 0.1>` -/// gets its own, which is the split morph#543 is about. +/// gets its own. /// /// @tparam T The payload's arithmetic type (`decltype(Min)`). template diff --git a/include/morph/forms/flows.hpp b/include/morph/forms/flows.hpp index 2c192d67..a0a5eb77 100644 --- a/include/morph/forms/flows.hpp +++ b/include/morph/forms/flows.hpp @@ -119,8 +119,8 @@ template step["title"] = std::string{StepT::title()}; if constexpr (std::tuple_size_v != 0) { // The insert is the point: `prefill` is being created here, not - // read, so this is one of the sites morph#706 leaves alone. A read - // would go through `morph::forms::detail::findMember` instead. + // read, so `operator[]` is the behaviour wanted. A read would go + // through `morph::forms::detail::findMember` instead. // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-avoid-unchecked-container-access) ::morph::forms::detail::emitBindsInto(step["prefill"]); } diff --git a/include/morph/forms/forms.hpp b/include/morph/forms/forms.hpp index 86317b61..97bc6a23 100644 --- a/include/morph/forms/forms.hpp +++ b/include/morph/forms/forms.hpp @@ -199,7 +199,7 @@ namespace morph::forms { /// declaration the schema advertises — the "the DTO *is* the form definition" /// rule (`examples/IMPLEMENTATION.md` rule 3) applied to a scalar bound, which /// the `formRules` vocabulary cannot express because every comparison node -/// there takes two member pointers, never a literal (morph#310). They are +/// there takes two member pointers, never a literal. They are /// keyed by field rather than by unit precisely so a floor declared for one /// `Quantity` member does not constrain a sibling of the same type — /// `UnitTraits::bounds` is per-unit and cannot make that distinction. @@ -1244,10 +1244,9 @@ struct Equals { node["value"] = literal; // An integral literal beyond 2^53 does not survive the renderer's // JSON.parse: it arrives rounded, and an `equals` against it then - // compares two values the schema kept distinct (morph#176). Carry - // the exact digits alongside, as `x-exactMinimum`/`x-exactMaximum` - // already do for bounds (morph#213); a renderer that ignores - // `valueText` behaves exactly as before. + // compares two values the schema kept distinct. Carry the exact + // digits alongside, as `x-exactMinimum`/`x-exactMaximum` do for + // bounds; a renderer that ignores `valueText` is unaffected. if constexpr (std::is_integral_v && !std::is_same_v) { if (std::cmp_greater(literal, kExactDoubleLimit) || std::cmp_less(literal, -kExactDoubleLimitSigned)) { // Glaze DOM builder — same shape as every sibling assignment here. @@ -2218,7 +2217,7 @@ void annotateBasicMemberProperty(glz::generic_u64& property, std::string_view na // then fires two mutually exclusive kind flags on it (`isBoolean` and // `isArray` both true) and draws a checkbox whose payload is a JSON array // of the string "false", reporting the form `ready` for a value nobody - // chose (morph#392). `glz::glaze_enum_t` is the same trait glaze's own + // chose. `glz::glaze_enum_t` is the same trait glaze's own // enum schema specialisation gates on, so this fires exactly when that // specialisation would not have been reached -- a `static_assert` whose // condition depends on @p Member, so it only fires for the specific @@ -2326,7 +2325,7 @@ void annotateBasicMemberProperty(glz::generic_u64& property, std::string_view na /// found.")`, i.e. by throwing. Which of the two a call gets is decided by the /// constness of the DOM, not by the spelling, which is why the remedy /// `cppcoreguidelines-pro-bounds-avoid-unchecked-container-access` suggests -/// cannot be adopted mechanically here (morph#706). +/// cannot be adopted mechanically here. /// /// This returns a pointer instead: absence is a value the caller branches on, /// a read never grows the document, and nothing throws. It also replaces the @@ -2436,21 +2435,21 @@ void recurseIntoNestedAggregateIfAny(SchemaDomRef dom, glz::generic_u64& propert /// is not instantiated again -- so a self-referential type /// (`struct Node { std::vector children; };`) or a mutual reference /// between two types costs one instantiation per type and stops, rather than -/// recursing forever (morph#703, measured: see -/// `docs/spec/forms/forms.md`, "Nested aggregates (recursive, cycle-safe)"). -/// -/// Two earlier designs are why this is worth stating. An ancestor *type list* -/// made every distinct root-to-member route through the type graph its own -/// instantiation, so a domain model that is a DAG rather than a tree -- an -/// `Address` under both a `Customer` and a `Supplier`, a `Money` everywhere -- -/// cost one instantiation per route, and route count grows exponentially in -/// the graph's size (morph#573, Part B: a fixture whose route count is -/// Fibonacci(n) reached 86 s at 2,584 routes). A depth counter (morph#573 -/// step 3) cut that to one instantiation per (type, depth) pair, at the price -/// of a 16-level cap and a `static_assert` that rejected every cyclic type. -/// Carrying nothing is strictly better than both: one instantiation per -/// *type*, no cap, and no rejection. The runtime recursion is stopped by -/// @p visited, not by the type system. +/// recursing forever (measured: see `docs/spec/forms/forms.md`, "Nested +/// aggregates (recursive, cycle-safe)"). +/// +/// Carrying *nothing* through the instantiation is what buys that, and the two +/// obvious alternatives both cost more. An ancestor *type list* makes every +/// distinct root-to-member route through the type graph its own instantiation, +/// so a domain model that is a DAG rather than a tree -- an `Address` under +/// both a `Customer` and a `Supplier`, a `Money` everywhere -- costs one +/// instantiation per route, and route count grows exponentially in the graph's +/// size (measured: a fixture whose route count is Fibonacci(n) reaches 86 s at +/// 2,584 routes). A depth counter cuts that to one instantiation per (type, +/// depth) pair, at the price of a depth cap and a `static_assert` that rejects +/// every cyclic type. Carrying nothing gives one instantiation per *type*, no +/// cap, and no rejection. The runtime recursion is stopped by @p visited, not +/// by the type system. /// /// morph therefore imposes no depth limit, but the *compiler* does, and MSVC's /// is low: 15 levels of nested aggregate initialisation inside an instantiated @@ -2476,8 +2475,7 @@ void recurseIntoNestedAggregateIfAny(SchemaDomRef dom, glz::generic_u64& propert // property glaze emitted without an `items` node is left alone // rather than given an empty one. The `contains` + `operator[]` // pair this replaces probed the same map twice and needed a - // standing suppression to say why the subscript was safe - // (morph#706). + // standing suppression to say why the subscript was safe. annotateNestedAggregateRef(dom, *items, visited); } } @@ -2576,7 +2574,7 @@ void annotateNestedAggregateRef(SchemaDomRef dom, glz::generic_u64& propertyOrIt // $defs key that doesn't exist, so this only changes behavior // for malformed input, which is left untouched instead. // findMember is what states that in the type system rather - // than in a comment beside a subscript (morph#706). + // than in a comment beside a subscript. auto* const defs = findMember(dom.value(), "$defs"); auto* const entry = (defs == nullptr) ? nullptr : findMember(*defs, key); // `visited.insert(...).second` is the first-arrival test: it @@ -2745,13 +2743,13 @@ template /// top-level `x-rules` array is such a conjunction (`allRulesSatisfied` folds /// it with `&&`) and so are an `and` node's `conditions`, so /// `ruleList(andOf(exactlyOneOf(&A::a, &A::b), engaged(&A::c)))` is exactly as -/// unsubmittable as the direct `ruleList(exactlyOneOf(&A::a, &A::b))` and is -/// now rejected too. It previously shipped, because the loop skipped any node -/// without a `fields` key and `and`/`or`/`not` emit `conditions`/`condition` -/// instead (morph#544) — and the identical contradiction being a hard build -/// failure in one spelling and a silently unsubmittable form in the other is -/// worse than not checking at all, since the check's existence is what an -/// author trusts. +/// unsubmittable as the direct `ruleList(exactlyOneOf(&A::a, &A::b))`, so it is +/// rejected too. Descending matters because `and`/`or`/`not` emit +/// `conditions`/`condition` rather than `fields`: a loop that skipped any node +/// without a `fields` key would let the wrapped spelling through. The identical +/// contradiction being a hard build failure in one spelling and a silently +/// unsubmittable form in the other is worse than not checking at all, since the +/// check's existence is what an author trusts. /// /// `or` and `not` are **not** descended, and that is not an omission: /// @@ -2804,9 +2802,8 @@ enum class ExactBoundKind : std::uint8_t { /// they are not rounded on the C++ side. They are rounded anyway the moment a /// renderer does `JSON.parse(controller.schemasJson)`, which every shipped app /// does -- `INT64_MAX` becomes `9223372036854775808`, and a client-side gate -/// comparing against it then admits `INT64_MAX + 1` as "not greater" -/// (morph#213). The exact digits travel as a string, which `JSON.parse` cannot -/// round. +/// comparing against it then admits `INT64_MAX + 1` as "not greater". The +/// exact digits travel as a string, which `JSON.parse` cannot round. /// /// Emitted only above `kExactDoubleLimit`: an ordinary bound loses nothing to a /// double, so schemas that do not need this are byte-for-byte unchanged. @@ -2819,7 +2816,7 @@ inline void annotateExactBound(glz::generic_u64& node, ExactBoundKind kind) { std::string_view const key = isMinimum ? "minimum" : "maximum"; std::string_view const textKey = isMinimum ? "x-exactMinimum" : "x-exactMaximum"; // A read, so it is checked: findMember yields nullptr for a node with no - // such bound instead of fabricating a null one (morph#706). The two + // such bound instead of fabricating a null one. The two // `node[textKey] =` writes further down are the opposite case -- the // companion key is *meant* to be created -- and keep `operator[]`, which // is what the suppression above is still for. @@ -3085,8 +3082,8 @@ template annotateSubmitMode(dom); - // Exact companions for any bound a double cannot hold (morph#213). Last, - // so it also covers nodes added by the passes above. + // Exact companions for any bound a double cannot hold. Last, so it also + // covers nodes added by the passes above. annotateExactNumericBounds(dom); // value_or without a move: the copy is irrelevant (schemaJson memoises), diff --git a/include/morph/forms/instance_constraints.hpp b/include/morph/forms/instance_constraints.hpp index c54d6979..e50b06d3 100644 --- a/include/morph/forms/instance_constraints.hpp +++ b/include/morph/forms/instance_constraints.hpp @@ -266,7 +266,7 @@ class InstanceConstraints { // Reads go through findMember, which answers "is it there?" and "where // is it?" in one probe and cannot grow the document on a miss; the // `x-*` writes below intend `operator[]`'s insert and keep it, which - // is what the suppression above is still for (morph#706). + // is what the suppression above is for. auto* const properties = detail::findMember(dom, "properties"); if (properties == nullptr) { return schema; diff --git a/include/morph/forms/views.hpp b/include/morph/forms/views.hpp index a96b3b68..b42a8aee 100644 --- a/include/morph/forms/views.hpp +++ b/include/morph/forms/views.hpp @@ -200,8 +200,9 @@ concept HasViewActions = requires { // deriveColumns, this function's only caller, has already returned "[]" // if it were absent. A findMember null check here would add a second // branch nothing can take -- untestable code and a future branch-coverage - // allowlist entry, which is the cost morph#706 warns against paying (see - // `docs/spec/forms/forms.md`, "Reading the DOM with `findMember`"). + // allowlist entry, which is a cost not worth paying for a check that can + // never fire (see `docs/spec/forms/forms.md`, "Reading the DOM with + // `findMember`"). auto const& propsObj = rowDom["properties"].get_object(); auto iter = propsObj.find(field); if (iter == propsObj.end()) { @@ -210,12 +211,11 @@ concept HasViewActions = requires { auto const& prop = iter->second; if (auto const* const decimals = ::morph::forms::detail::findMember(prop, "x-decimalPlaces")) { // A *write* into the entry being built: `operator[]`'s insert is the - // behaviour wanted, exactly as morph#706 left `forms.hpp`'s writes - // alone. Converting a write to a null check would change behaviour - // rather than make it safe. The directive is here, and on the two - // below, only because converting the read beside it moved this line - // into the diff, and `clang-tidy-diff` reports on changed lines -- - // the piecemeal bill morph#677 describes. + // behaviour wanted, exactly as for `forms.hpp`'s writes. Converting a + // write to a null check would change behaviour rather than make it + // safe. The directive is here, and on the two below, because + // `clang-tidy-diff` reports on changed lines and these lines sit in + // the diff alongside the read beside them. // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-avoid-unchecked-container-access) entry["x-decimalPlaces"] = *decimals; } diff --git a/include/morph/forms/widget_hints.hpp b/include/morph/forms/widget_hints.hpp index d95f26c8..06fbbf59 100644 --- a/include/morph/forms/widget_hints.hpp +++ b/include/morph/forms/widget_hints.hpp @@ -136,9 +136,9 @@ struct glz::meta { /// `$defs` entry describes — the bounds are emitted as property-level /// `x-min`/`x-max`/`x-step` and never reach the definition. With one shared /// `"Ranged"` name, a `double` slider and an `int` slider in the same action -/// collapsed into a single entry whose type was wrong for one of them -/// (morph#543); two `Ranged` fields of the *same* payload type still share one -/// entry, because their entries are identical. See +/// would collapse into a single entry whose type is wrong for one of them. +/// Two `Ranged` fields of the *same* payload type do share one entry, because +/// their entries are identical. See /// `forms/detail/schema_name.hpp`. /// @tparam Min Inclusive lower bound. /// @tparam Max Inclusive upper bound. diff --git a/include/morph/net/detail/tcp_socket.hpp b/include/morph/net/detail/tcp_socket.hpp index ea184d2d..d9365d9c 100644 --- a/include/morph/net/detail/tcp_socket.hpp +++ b/include/morph/net/detail/tcp_socket.hpp @@ -48,12 +48,12 @@ class TcpSocket { /// `sendAll()` are written for blocking descriptors — neither treats /// `EAGAIN` as anything but a fatal error. /// - /// The blocking reset is not hypothetical bookkeeping (morph#478). + /// The blocking reset is not hypothetical bookkeeping. /// macOS/BSD propagate a listening socket's `O_NONBLOCK` onto the sockets /// `accept(2)` returns; POSIX permits that and Linux documents that it does - /// not do it. Since morph#437 `SocketServer::listen()` makes its listener - /// non-blocking, so without this every connection `tryAccept()` handed back - /// on macOS/BSD was non-blocking too, and `clientLoop()`'s first read — + /// not do it. `SocketServer::listen()` makes its listener non-blocking, so + /// without this every connection `tryAccept()` hands back on macOS/BSD + /// would be non-blocking too, and `clientLoop()`'s first read — /// `performServerHandshake()` — threw on `EAGAIN` before the client's /// Upgrade request had arrived. Every connection failed, and Linux-only CI /// could not see it. Clearing it here rather than in `tryAccept()` covers @@ -111,13 +111,13 @@ class TcpSocket { if (rc != 0 || resolved == nullptr) { // `::gai_strerror`, and NOT `errnoMessage()`: `rc` is an `EAI_*` // code, not an `errno`, so `std::system_category().message(rc)` - // would render a confidently wrong string (morph#641). + // would render a confidently wrong string. // - // It also stays here rather than going the way `std::strerror` - // went in morph#625, because it does not have `std::strerror`'s + // It also stays, rather than being replaced the way `std::strerror` + // was, because it does not have `std::strerror`'s thread-safety // defect. This throw site runs on threads this subsystem spawns - // (see `errnoMessage` below), so the question was live; it was - // measured rather than assumed (morph#640). + // (see `errnoMessage` below), so the question is live; what follows + // is measured rather than assumed. // // glibc 2.44, `gcc -O0`: `gai_strerror` returns a pointer to a // string literal inside libc's own read-only data, distinct per @@ -147,9 +147,9 @@ class TcpSocket { // One deadline for the whole call, not one timeout per candidate. // `ai_family = AF_UNSPEC` makes several candidates the norm ("localhost" // resolves to both ::1 and 127.0.0.1), and polling `timeout` inside the - // loop meant the worst case was N x timeout -- while two doc comments - // (here and SocketBackend's destructor note) state it as a single bound. - // morph#507. + // loop would make the worst case N x timeout -- while two doc comments + // (here and SocketBackend's destructor note) state it as a single + // bound. A deadline computed once keeps the stated bound true. auto const deadline = std::chrono::steady_clock::now() + timeout; for (addrinfo* rp = resolved; rp != nullptr; rp = rp->ai_next) { @@ -259,7 +259,7 @@ class TcpSocket { /// **This call has no portable interruption mechanism.** `shutdown(2)` on a /// *listening* socket unblocks a parked `accept()` on Linux, but that is a /// Linux property rather than a POSIX one — on macOS/BSD the parked thread - /// stays parked (morph#437). Anything that has to be able to stop waiting + /// stays parked. Anything that has to be able to stop waiting /// must therefore not park here at all: use `setNonBlocking()` plus /// `::poll` on `nativeHandle()` alongside a self-pipe, and take the /// connection with `tryAccept()`. `SocketServer::acceptLoop()` is the @@ -279,16 +279,12 @@ class TcpSocket { // SIGCHLD, SIGWINCH) tears down the accept loop and the server // silently stops taking connections. // - // Every other errno throws, and no other one is retried. This - // comment used to single out ECONNABORTED as "deliberately not - // retried, because shutdownBoth() is the documented way to break - // out of this call" -- which named the wrong errno for the - // mechanism it was guarding (morph#465): `shutdown(listenfd, - // SHUT_RDWR)` unblocks a parked `accept()` with EINVAL, not - // ECONNABORTED. Since morph#437 it is doubly stale, because - // shutting the listener down is no longer how anything breaks out - // of an accept loop -- `SocketServer` polls a self-pipe instead and - // never parks here at all. + // Every other errno throws, and no other one is retried. + // ECONNABORTED in particular is not special-cased: `shutdown( + // listenfd, SHUT_RDWR)` unblocks a parked `accept()` with EINVAL, + // not ECONNABORTED, and shutting the listener down is not how + // anything here breaks out of an accept loop -- `SocketServer` + // polls a self-pipe instead and never parks here at all. // // No ECONNABORTED retry is added in its place. A reset-race repro // over six shapes (blocking and non-blocking + poll, with and @@ -335,8 +331,8 @@ class TcpSocket { /// The connection it yields is **blocking**, whatever mode this listener is /// in: the fd-adopting constructor clears `O_NONBLOCK`, which is what stops /// macOS/BSD's inheritance of the listener's flag reaching `recvSome()` - /// (morph#478). Any rewrite of this function has to keep going through that - /// constructor, or restore the reset itself. + /// Any rewrite of this function has to keep going through that + /// constructor, or do the reset itself. /// @return The accepted `TcpSocket`, or `std::nullopt` when no connection /// was pending — a readiness report that went stale before the /// `accept`, which the caller answers by waiting again. @@ -406,8 +402,8 @@ class TcpSocket { /// /// Without this a `sendAll` against a peer that has stopped reading blocks /// forever once the kernel send buffer fills, and it does so while holding - /// whatever lock its caller took -- which is how `~SocketBackend` came to be - /// parkable behind `_socketMtx` (morph#506). With `SO_SNDTIMEO` set, the + /// whatever lock its caller took -- which is how `~SocketBackend` becomes + /// parkable behind `_socketMtx`. With `SO_SNDTIMEO` set, the /// blocked `send` returns `EAGAIN`/`EWOULDBLOCK` instead, `sendAll` throws /// as it already does for any other send error, and the lock is released. /// @@ -436,7 +432,7 @@ class TcpSocket { /// the caller in `recvSome` forever -- for `SocketBackend`'s client /// handshake read specifically, that in turn wedges `~SocketBackend`, /// since the destructor's escape hatch only reaches a socket already - /// published to `_socket` (morph#535). + /// published to `_socket`. /// /// A timed-out `recv` makes `recvSome` throw (`EAGAIN`/`EWOULDBLOCK` is /// not one of the errors it treats as an orderly close), so callers meant @@ -457,7 +453,7 @@ class TcpSocket { /// /// Documented for *connected* sockets only, which is where `shutdown(2)` /// waking a parked peer call is portable. It is not a way to interrupt - /// `accept()` on a listening socket — see `accept()` and morph#437. + /// `accept()` on a listening socket — see `accept()`. void shutdownBoth() noexcept { if (_fd >= 0) { ::shutdown(_fd, SHUT_RDWR); @@ -492,8 +488,9 @@ class TcpSocket { /// values are `errno` values on POSIX, which is what every caller here /// passes. Preferred over `strerror_r` because that function's XSI and GNU /// variants differ in return type, so a portable call needs a build-time - /// discriminator and a caller-supplied buffer; this needs neither. - /// morph#625. + /// discriminator and a caller-supplied buffer; this needs neither. Also + /// preferred over `std::strerror`, which is not thread-safe and this + /// subsystem calls it from threads it spawns. static std::string errnoMessage(int err) { return std::system_category().message(err); } /// POSIX allows `EAGAIN` and `EWOULDBLOCK` to differ, and both name the diff --git a/include/morph/net/socket_backend.hpp b/include/morph/net/socket_backend.hpp index b17aceee..ac7fd8e9 100644 --- a/include/morph/net/socket_backend.hpp +++ b/include/morph/net/socket_backend.hpp @@ -41,7 +41,7 @@ struct SocketBackendConfig { /// Applied as `SO_SNDTIMEO`. Without it, a peer that stops reading fills the /// kernel send buffer and parks `sendFrame` inside `sendAll` **while holding /// `_socketMtx`** -- which parks `~SocketBackend` behind the same lock, with - /// nothing able to release it (morph#506). Generous on purpose: it bounds a + /// nothing able to release it. Generous on purpose: it bounds a /// send making *no* progress, not a slow one. Zero disables it. std::chrono::milliseconds sendTimeout{30000}; /// @brief Bound on the handshake response read that follows a successful @@ -56,7 +56,7 @@ struct SocketBackendConfig { /// parked in a blocking `recv` with nothing to unblock it, which in turn /// wedges `~SocketBackend` forever: the destructor's escape hatch only /// reaches a socket already published to `_socket`, and that publish - /// happens only *after* the handshake (morph#535). Zero disables it + /// happens only *after* the handshake. Zero disables it /// (the kernel default, block forever). /// /// @warning `SO_RCVTIMEO` restarts on every `recv`, so this bounds each @@ -116,21 +116,20 @@ class SocketBackend : public ::morph::backend::detail::IBackend { /// rather than the handshake as a whole, so a peer that dribbles one /// header byte per interval can stretch that phase to roughly /// `handshakeTimeout` times the 64 KiB header cap. Against a peer that - /// simply stops writing (the case morph#535 is about) the bound is one + /// simply stops writing, the bound is one /// `handshakeTimeout`. See `docs/spec/core/backend.md`'s `morph::net` /// section. ~SocketBackend() override { _shuttingDown.store(true); - // Under `_socketMtx`, and it has to be -- see morph#506, which proposed - // dropping it and was proved wrong by ThreadSanitizer. `shutdownBoth()` - // is indeed safe to call from any thread, but that is not what the lock - // is protecting here: `onDisconnected()` *reassigns* `_socket` - // (`_socket = TcpSocket{}`, a move-assign that closes the old fd), so an - // unlocked `_socket.valid()` here races the I/O thread replacing the - // object out from under it. + // Under `_socketMtx`, and it has to be -- dropping the lock here is + // what ThreadSanitizer flags. `shutdownBoth()` is indeed safe to call + // from any thread, but that is not what the lock is protecting: + // `onDisconnected()` *reassigns* `_socket` (`_socket = TcpSocket{}`, a + // move-assign that closes the old fd), so an unlocked `_socket.valid()` + // here races the I/O thread replacing the object out from under it. // - // The hazard #506 describes is closed from the other end: `sendAll` is - // no longer un-timed. `Config::sendTimeout` (SO_SNDTIMEO, 30s default) + // The risk of parking behind this lock is closed from the other end: + // `sendAll` is not un-timed. `Config::sendTimeout` (SO_SNDTIMEO, 30s default) // bounds any single send that makes no progress, so a peer that stops // reading can hold `_socketMtx` for at most that long instead of // forever, and this wait is bounded rather than open-ended. Fixing it @@ -203,7 +202,7 @@ class SocketBackend : public ::morph::backend::detail::IBackend { /// reaches it. `RemoteServer::attachLogIfConfigured` returns without /// consulting its `LogProvider` at all when the envelope's `contextKey` is /// empty, so dropping it here does not merely lose an entity key — it leaves - /// the instance unjournalled (morph#587). `SimulatedRemoteBackend` overrides + /// the instance unjournalled. `SimulatedRemoteBackend` overrides /// this for the same reason; the two must not disagree. /// /// Same synchronous-call constraint as `registerModel`. The factory argument @@ -278,7 +277,7 @@ class SocketBackend : public ::morph::backend::detail::IBackend { (void)sendControlForId(env, "assign"); } - // ── The structural registration surface (morph#567 / morph#569) ────── + // ── The structural registration surface ────────────────────────────── // // Overridden natively rather than reached through // `backend::SynchronousBackendAdapter`. The reasoning is recorded in @@ -293,10 +292,9 @@ class SocketBackend : public ::morph::backend::detail::IBackend { // which this backend is otherwise documented as not having (it may be // driven from several threads at once). // - // Nothing about the legacy verbs changes: `registerModel`, - // `registerModelShared`, `attachModel` and `assignPrimary` still use - // `sendSync` and `callId == 0`, so every caller morph#570/#571 has yet to - // migrate behaves exactly as before. + // The synchronous verbs are unaffected: `registerModel`, + // `registerModelShared`, `attachModel` and `assignPrimary` use `sendSync` + // and `callId == 0`. /// @brief Acquires a model instance without blocking the calling thread. /// @@ -392,11 +390,10 @@ class SocketBackend : public ::morph::backend::detail::IBackend { /// @brief Sends a `deregister` message fire-and-forget (does not wait for a reply). /// /// Carries a real, non-zero `callId` drawn from the same shared counter - /// (`_pending.nextCallId()`) `execute()` uses, exactly as `QtWebSocketBackend` does (see - /// issue #65, and #454 for this transport's own reoccurrence of it): + /// (`_pending.nextCallId()`) `execute()` uses, exactly as `QtWebSocketBackend` does: /// `callId == 0` is `dispatchIncomingEnvelope`'s discriminator for "hand /// this payload to whichever `sendSync()` is parked", so a - /// fire-and-forget `deregister` sharing that sentinel had its own stray + /// fire-and-forget `deregister` sharing that sentinel would have its own stray /// `ok` reply delivered to an unrelated `register`/`attach` waiting on /// `_syncCv` whenever the two landed back to back on one connection. /// @@ -482,8 +479,8 @@ class SocketBackend : public ::morph::backend::detail::IBackend { sendFrame(::morph::net::detail::WsOpcode::kText, ::morph::wire::encode(env)); } catch (const std::exception&) { // Either the write raced an already-in-progress disconnect, or - // (morph#536) `sendFrame` itself just tore the connection down - // after a failed/partial send. Either way a disconnect is now + // `sendFrame` itself just tore the connection down after a + // failed/partial send. Either way a disconnect is now // underway, and the io thread's handler drains _pending // (including this entry) via cancelPending. } @@ -644,7 +641,7 @@ class SocketBackend : public ::morph::backend::detail::IBackend { // `sendAll` can throw having already written part of the frame // (e.g. `SO_SNDTIMEO` firing mid-send) -- the peer's frame stream // is now desynchronised, and a subsequent send here would append - // a fresh frame into the middle of the truncated one (morph#536). + // a fresh frame into the middle of the truncated one. // Every caller of `sendFrame` reaches this one lock, so tearing // the connection down *here* -- rather than in each of them -- // is enough to cover them all: `shutdownBoth()` unblocks the io @@ -911,15 +908,15 @@ class SocketBackend : public ::morph::backend::detail::IBackend { // Before the handshake, so even that cannot park forever. // Bounds any single send that makes no progress, which is // what keeps ~SocketBackend from being parked behind - // _socketMtx by a peer that stopped reading (morph#506). + // _socketMtx by a peer that stopped reading. (void)socket.setSendTimeout(_cfg.sendTimeout); } if (_cfg.handshakeTimeout.count() > 0) { // Bounds the handshake response read, which otherwise has // no timeout of its own and can park this thread forever - // against a peer that accepts and then stays silent - // (morph#535) -- and this fd is not yet published to - // `_socket`, so the destructor cannot reach it either. + // against a peer that accepts and then stays silent -- and + // this fd is not yet published to `_socket`, so the + // destructor cannot reach it either. (void)socket.setRecvTimeout(_cfg.handshakeTimeout); } std::string leftover = ::morph::net::detail::performClientHandshake(socket, _url); diff --git a/include/morph/net/socket_server.hpp b/include/morph/net/socket_server.hpp index c036c6d8..2464ed67 100644 --- a/include/morph/net/socket_server.hpp +++ b/include/morph/net/socket_server.hpp @@ -115,7 +115,7 @@ class SocketServer { /// ordering"): no lock inside this object can outlive the object holding /// it. void close() { - // Serialize the whole body, not just the guard below (morph#451). + // Serialize the whole body, not just the guard below. // `_closing.exchange` alone cannot exclude a second caller: the loser // still sees a *joinable* accept thread — the winner has not joined it // yet and cannot have, since that thread is parked in poll() until the @@ -139,10 +139,10 @@ class SocketServer { return; } // The accept loop's wakeup, and the reason this teardown terminates at - // all (morph#437). It used to be `_listenSocket.shutdownBoth()`, which - // works only because Linux happens to kick a parked accept(2) when the - // listening socket is shut down -- macOS/BSD do not, so the join below - // never returned there. Nothing about that was arranged by this code. + // all. `_listenSocket.shutdownBoth()` would not do: it works only + // because Linux happens to kick a parked accept(2) when the listening + // socket is shut down, and macOS/BSD do not, so the join below would + // never return there. // The loop now parks in poll() on this pipe as well as on the listener, // so one byte here ends it on every platform. // @@ -204,7 +204,7 @@ class SocketServer { std::atomic closed{false}; /// Set by `clientLoop` as its last act, so `acceptLoop` can tell a /// finished connection from a live one and reclaim both its fd and its - /// thread handle. See `reapFinishedClients` (morph#498). + /// thread handle. See `reapFinishedClients`. std::atomic finished{false}; /// Writes one reply frame. A failure is never propagated to the caller @@ -216,9 +216,9 @@ class SocketServer { /// *writes* -- `clientLoop()` is blocked in `recvSome()` and never /// consults it, so without the `shutdownBoth()` the connection goes on /// draining and dispatching whatever the peer already queued, into a - /// `RemoteServer` whose replies this function then silently drops - /// (morph#536: *any* caller observing a partial write marks the - /// connection unusable, and this is one of them). + /// `RemoteServer` whose replies this function then silently drops. + /// The rule is that *any* caller observing a partial write marks the + /// connection unusable, and this is one of them. void sendText(const std::string& payload) { std::scoped_lock lock{writeMtx}; if (closed.load() || !socket.valid()) { @@ -270,10 +270,10 @@ class SocketServer { if (_closing.load()) { return; } - // Before taking on another one: nothing else ever removed a - // finished connection, so an fd and a joinable thread handle - // accumulated per connection *ever accepted*, not per live - // connection, until close() (morph#498). + // Before taking on another one: nothing else removes a finished + // connection, so without this an fd and a joinable thread handle + // accumulate per connection *ever accepted*, not per live + // connection, until close(). reapFinishedClients(); auto conn = std::make_shared(std::move(*clientSocket), _server.openConnection()); @@ -322,7 +322,7 @@ class SocketServer { // reaper can release the fd and join the thread handle rather than // holding both until close(). Declared *before* the scope guard below so // it is destroyed last: the flag must not go up until the connection's - // models have actually been reclaimed. morph#498. + // models have actually been reclaimed. struct FinishedFlag { explicit FinishedFlag(std::atomic& target MORPH_LIFETIMEBOUND) : flag{target} {} ~FinishedFlag() { flag.store(true, std::memory_order_release); } @@ -393,7 +393,7 @@ class SocketServer { /// closing it: the read side keeps working, so a future frame written /// here would land in the middle of the truncated one. `closed` is /// therefore set exactly as `sendText()`'s own catch does, so no later - /// write on this connection is attempted (morph#536). + /// write on this connection is attempted. /// /// Marking it closed is not enough on its own, though: `closed` only gates /// *writes*, and `clientLoop()` is blocked in `recvSome()` on a socket the @@ -460,7 +460,7 @@ class SocketServer { } /// RAII owner of the self-pipe `acceptLoop()` polls alongside the listener - /// and `close()` writes one byte to (morph#437). + /// and `close()` writes one byte to. /// /// A pipe rather than an `eventfd`: `eventfd` is Linux-only, and the whole /// point of this mechanism is that it is the *same* mechanism on every @@ -557,7 +557,7 @@ class SocketServer { /// joined, so the two never touch it concurrently. WakeupPipe _wakeup; /// Serializes `close()` against itself so only one caller ever reaches - /// `_acceptThread.join()` (morph#451). Not taken anywhere else. + /// `_acceptThread.join()`. Not taken anywhere else. std::mutex _closeMtx; std::atomic _closing{true}; std::thread _acceptThread; diff --git a/include/morph/offline/file_offline_queue.hpp b/include/morph/offline/file_offline_queue.hpp index c1e0e7dd..40e0f595 100644 --- a/include/morph/offline/file_offline_queue.hpp +++ b/include/morph/offline/file_offline_queue.hpp @@ -159,17 +159,14 @@ class FileOfflineQueue : public IOfflineQueue { explicit FileOfflineQueue(std::filesystem::path path, ::morph::core::FileIoOps ioOps = {}, std::optional maxDepth = std::nullopt) : _path{std::move(path)}, _io{std::move(ioOps)}, _maxDepth{maxDepth} { - // No constructor-time repairTornTail() here, deliberately. + // No constructor-time repairTornTail() here, deliberately. Calling it + // before load() would not heal an "interior merge from a doubled-up + // short write": repairTornTail only trims bytes after the final + // newline, and says so itself -- "Complete records, including a + // malformed *interior* line, are left exactly as they are." // - // An earlier revision of morph#530 called it before load(), to heal an - // "interior merge from a doubled-up short write" that load() would - // otherwise reject. It cannot do that: repairTornTail only trims bytes - // after the final newline, and says so itself -- "Complete records, - // including a malformed *interior* line, are left exactly as they are." - // So it never fixed the case it was added for. - // - // It did break two things. It is the constructor's only file mutation - // that can run *before* load() throws, which costs morph#494's + // It would also break two things. It would be the constructor's only + // file mutation that can run *before* load() throws, costing the // guarantee that a failed construction leaves the queue file // byte-identical (a file with both a malformed interior line and a torn // tail would come back truncated *and* throw). And it discards a @@ -177,11 +174,11 @@ class FileOfflineQueue : public IOfflineQueue { // -- which load() decodes perfectly well -- wiping the file outright // when that is the only line. // - // What actually prevents the doubled-up short write is the rollback in + // What prevents the doubled-up short write is the rollback in // writeLine() below, which leaves no partial bytes for a later write to - // merge with; load()+compact() heal an ordinary torn tail as they always - // have. FileActionLog keeps its own long-standing call: that is - // pre-existing behaviour there, not something this change introduced. + // merge with; load()+compact() heal an ordinary torn tail. + // FileActionLog calls repairTornTail at construction because its own + // file shape makes that safe there. load(); compact(); _file = _io.fopen(_path.string(), "a"); @@ -351,10 +348,11 @@ class FileOfflineQueue : public IOfflineQueue { // so fwrite is a memcpy into the stdio buffer and returns the full // count even on a full disk; the write(2) that actually fails happens // inside syncFile's fflush. Wired to the short-write branch alone, an - // ENOSPC there threw with a truncated line already on disk, at exactly - // the offset the next writeLine resumes from and with no separating - // newline -- the identical merge morph#530 exists to prevent, and the - // *common* manifestation of a full disk rather than an exotic one. + // ENOSPC there would throw with a truncated line already on disk, at + // exactly the offset the next writeLine resumes from and with no + // separating newline -- the identical merge the rollback exists to + // prevent, and the *common* manifestation of a full disk rather than + // an exotic one. auto const rollBackAndThrow = [&](const std::string& what) { if (::morph::core::rollBackShortWrite(_io, _file, _path, offsetBeforeWrite) == ::morph::core::RollBack::torn) { @@ -377,7 +375,7 @@ class FileOfflineQueue : public IOfflineQueue { // no separating newline -- merging into one line load() can only // tolerate while it stays the trailing line, and stops being able // to the moment a further write pushes it into an interior position - // (morph#530). Roll the file back to its pre-write length instead, + // at all. Roll the file back to its pre-write length instead, // so a failed write leaves no trace at all for the next one to // merge with. Best-effort: this is already the failure path, and // when the rollback's own flush cannot complete (the disk that made @@ -565,7 +563,7 @@ class FileOfflineQueue : public IOfflineQueue { // The rename is a directory mutation, not a file-content one -- fsync // on `out` above made the compacted *data* durable, but not the // directory entry that now names it `_path` instead of the tmp name - // (morph#532). Surfaced rather than swallowed, same as every other + // durable. Surfaced rather than swallowed, same as every other // fsync failure in this class; safe to throw here, since compact() // always runs before `_file` is opened -- nothing left dangling. auto const dirSync = ::morph::core::classifyDirectorySync(_io.syncPath(_path.parent_path())); diff --git a/include/morph/offline/replay_ledger.hpp b/include/morph/offline/replay_ledger.hpp index aa996018..38960d97 100644 --- a/include/morph/offline/replay_ledger.hpp +++ b/include/morph/offline/replay_ledger.hpp @@ -14,8 +14,8 @@ namespace morph::offline { /// /// Answers one question — "has this operation id already been applied?" — /// for a host that must dedup a retried write against one it already -/// committed. Promoted from seven hand-written, near-identical copies of the -/// same table across five example rungs (morph#226): `kanban`/`ledger` store a +/// committed. One framework seam in place of the near-identical table five +/// example rungs would each hand-write: `kanban`/`ledger` store a /// result and replay it verbatim on a hit ("response-replay"); `lims`/ /// `bookmarks`/`ledger`'s import path store nothing and report only that the /// op was seen ("skip-only"). Both are the same mechanism with a different @@ -93,8 +93,7 @@ struct IReplayLedger { /// construction), never by opening a connection of its own. Recording /// outside that transaction reintroduces exactly the defect this ledger /// exists to prevent: a crash between the write and the record redelivers - /// the operation and it is re-applied (morph#458 was this defect, shipped - /// in two rungs, before this interface existed). + /// the operation and it is re-applied. /// /// @par Idempotent: first-write-wins /// Recording an @p opId within @p scope that is already decided is a @@ -159,12 +158,11 @@ class InMemoryReplayLedger : public IReplayLedger { protected: /// @brief Looks up @p opId within @p scope in the in-memory map. /// - /// @par The two `std::string` constructions below are deliberate (morph#728) + /// @par The two `std::string` constructions below are deliberate /// They materialise the key inside the lock purely to probe an ordered map - /// that could take a transparent comparator instead. That is morph#699's - /// family, and it was profiled rather than fixed for symmetry. Measured on - /// `d03c66f3`, clang 22 `-O2`, a counting `operator new`, 2e6 iterations - /// per row: + /// that could take a transparent comparator instead. That was profiled + /// rather than fixed. Measured with clang 22 `-O2`, a counting + /// `operator new`, 2e6 iterations per row: /// /// @verbatim /// -- allocations per lookup() -- @@ -181,9 +179,9 @@ class InMemoryReplayLedger : public IReplayLedger { /// both past SSO : 740.3 ns /// @endverbatim /// - /// So the mechanism is real and id-length-dependent exactly as morph#699 - /// found, and the widened critical section costs ~55 ns of a ~740 ns - /// contended lookup. What parks it is the call census, not the size: + /// So the cost is real and id-length-dependent, and the widened critical + /// section costs ~55 ns of a ~740 ns contended lookup. What parks it is + /// the call census, not the size: /// `InMemoryReplayLedger` is constructed in **one** file in this tree, /// `tests/test_replay_ledger.cpp`, and in no shipping code at all. The /// only production `IReplayLedger::lookup()` call @@ -192,10 +190,9 @@ class InMemoryReplayLedger : public IReplayLedger { /// `doLookup` is an ODBC round-trip -- next to which 3 ns is not /// measurable. /// - /// This is morph#709's disposition, for morph#709's reason. It becomes - /// worth fixing the moment a per-request caller of *this* class appears; - /// the fix is then a transparent comparator on `_entries`, `std::map`'s - /// `is_transparent` flavour rather than `core/registry.hpp`'s + /// It becomes worth fixing the moment a per-request caller of *this* class + /// appears; the fix is then a transparent comparator on `_entries`, + /// `std::map`'s `is_transparent` flavour rather than `core/registry.hpp`'s /// hash-and-equality pair (which serves an `unordered_map` and does not /// apply here). /// diff --git a/include/morph/offline/sqlite_offline_queue.hpp b/include/morph/offline/sqlite_offline_queue.hpp index 48ac6570..d74131fd 100644 --- a/include/morph/offline/sqlite_offline_queue.hpp +++ b/include/morph/offline/sqlite_offline_queue.hpp @@ -111,7 +111,7 @@ class SqliteOfflineQueue : public IOfflineQueue { public: using IOfflineQueue::enqueue; // keep the two-arg overload visible - /// @brief `PRAGMA busy_timeout` set at construction (morph#532): how long + /// @brief `PRAGMA busy_timeout` set at construction: how long /// a statement blocks on `SQLITE_BUSY` before giving up, in /// milliseconds. static constexpr int kBusyTimeoutMillis = 5000; @@ -238,7 +238,7 @@ class SqliteOfflineQueue : public IOfflineQueue { execOrThrow("PRAGMA journal_mode=WAL;"); // execOrThrow() discards sqlite3_exec's row callback, so a silent - // fallback would otherwise go unnoticed (morph#532). Read the + // fallback would otherwise go unnoticed. Read the // pragma back through a real prepared statement rather than // trusting the set. // @@ -282,9 +282,9 @@ class SqliteOfflineQueue : public IOfflineQueue { // sqlite3_open() above creates `_path` (and, once journal_mode=WAL // took, its "-wal"/"-shm" siblings) if it did not already exist -- // a fresh directory entry that SQLite's own internal fsyncs of the - // *file's* contents never make durable, the identical - // directory-vs-file-fsync gap morph#532 closed for - // `FileActionLog`/`FileOfflineQueue` (see `FileIoOps::syncPath`'s + // *file's* contents never make durable -- the identical + // directory-vs-file-fsync gap + // `FileActionLog`/`FileOfflineQueue` close (see `FileIoOps::syncPath`'s // own docs). Unconditional: harmless when the file already // existed, since syncing an unchanged directory is a cheap no-op. // @@ -495,11 +495,11 @@ class SqliteOfflineQueue : public IOfflineQueue { std::scoped_lock const lock{_mtx}; // `WHERE NOT EXISTS (...)` rather than a bare UPDATE: the partial // unique index `ix_queue_idem` rejects stamping a non-empty key a - // pending row already holds, and a bare UPDATE turned that into a + // pending row already holds, and a bare UPDATE would turn that into a // thrown SqliteOfflineQueueError -- while this class's own // `enqueue(payload, key)` resolves the identical conflict silently, by - // keeping the existing row (morph#249). Same conflict, two answers, - // and only one of them matched the documented dedup contract. + // keeping the existing row. Same conflict, two answers, and only one of + // them matches the documented dedup contract. // // Throwing also bought nothing. This hook is called by the *base* // `IOfflineQueue::enqueue(payload, key)` default, which has already @@ -548,7 +548,7 @@ class SqliteOfflineQueue : public IOfflineQueue { void bindText(sqlite3_stmt* stmt, int index, const std::string& value) const { // An explicit length (not -1) is required so a NUL inside `value` -- // legitimate, since payload/idempotencyKey are opaque strings the - // caller controls the serialisation of (morph#531) -- doesn't tell + // caller controls the serialisation of -- doesn't tell // SQLite to measure only up to that byte and silently truncate. if (value.size() > static_cast(INT_MAX)) { throw SqliteOfflineQueueError{"SqliteOfflineQueue: value exceeds INT_MAX bytes"}; @@ -581,9 +581,8 @@ class SqliteOfflineQueue : public IOfflineQueue { // sqlite3_column_bytes() gives the real stored length; constructing a // std::string from the raw `const char*` alone would stop at the // first NUL and silently truncate a NUL-bearing payload or - // idempotency key on the way back out (morph#531) -- the read-side - // half of the same truncation bindText() above fixes on the write - // side. + // idempotency key on the way back out -- the read-side half of the + // same truncation bindText() above prevents on the write side. const auto* text = reinterpret_cast(sqlite3_column_text(stmt, index)); return text != nullptr ? std::string{text, static_cast(sqlite3_column_bytes(stmt, index))} : std::string{}; diff --git a/include/morph/util/quantity.hpp b/include/morph/util/quantity.hpp index de0da57f..ea4497c9 100644 --- a/include/morph/util/quantity.hpp +++ b/include/morph/util/quantity.hpp @@ -41,8 +41,8 @@ /// `named()` discards the name — so a build that never set it must not have /// that output emptied underneath it. The cost is real and measured, though: /// a 200,000-iteration running total took 54,056 KB and 0.034 s with -/// provenance against 12,236 KB and 0.006 s without (morph#574, clang 22, -/// `-O2`). **A bulk path that never calls `equation()` should set this to `0`.** +/// provenance against 12,236 KB and 0.006 s without (clang 22, `-O2`). +/// **A bulk path that never calls `equation()` should set this to `0`.** /// See `docs/spec/util/quantity_type.md`, *Limitations*. #ifndef MORPH_QUANTITY_PROVENANCE #define MORPH_QUANTITY_PROVENANCE 1 @@ -109,7 +109,7 @@ namespace detail { // operation, and INT64_MIN reaches here through the whole-integer // `Rational{value, DecimalPlaces{n}}` constructor, which does not // canonicalise (and `numerator` is public). `absU64` is the shared helper - // that gets this right -- see morph#496. + // that gets this right. auto const num = ::morph::math::detail::absU64(value.numerator); auto const den = static_cast(value.denominator); auto const places = static_cast(value.decimalPlaces.value); @@ -475,8 +475,8 @@ concept SameEnumDistinct = std::same_as && (A != B); /// /// A derivation has no bound — `total = total + row` over a data-driven loop /// records one step per iteration — so without a limit `equation()` renders -/// every one of them into a single line (morph#582: 100,000 steps produced a -/// 500,001-character line in 58.8 s). This is where a *rendered* formula stops +/// every one of them into a single line — measured at 100,000 steps: a +/// 500,001-character line in 58.8 s. This is where a *rendered* formula stops /// being something a person reads, not where the cost stops being tolerable: /// a caller that wants more passes its own limit, so erring low costs one /// argument while erring high costs an unreadable line nobody asked for. @@ -484,8 +484,8 @@ inline constexpr std::size_t kDefaultEquationSteps = 100; /// @brief Pass as `equation()`'s limit to write the derivation out in full. /// -/// Restores the pre-morph#582 behaviour: no step is elided, and the cost is -/// the caller's, taken deliberately. Used by the depth regression tests, which +/// No step is elided, and the cost is the caller's, taken deliberately. Used +/// by the depth regression tests, which /// exist precisely to walk a derivation deeper than any limit would render. inline constexpr std::size_t kEquationStepsUnlimited = std::numeric_limits::max(); @@ -584,8 +584,8 @@ struct ASTNode { /// derivation that is a linear chain down `left`, one node per iteration. /// The compiler-generated destructor releases that chain recursively — /// `~shared_ptr` -> `~ASTNode` -> `~shared_ptr` -> ... — one stack frame - /// per node, and a long enough chain runs the stack out. Measured - /// (morph#574, clang 22, `-O0`, 8 MiB stack): a 21,000-node chain + /// per node, and a long enough chain runs the stack out. Measured with + /// clang 22, `-O0`, an 8 MiB stack: a 21,000-node chain /// segfaults on destruction, while an optimised build survives 200,000 /// because clang turns the same chain into a loop. "Crashes in Debug, /// survives in Release" is the worst signature a defect can have, so the From 8c81cb702bfe33ad5a0902d8b63308bc88ed001c Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 23 Sep 2026 18:41:07 +0200 Subject: [PATCH 5/5] comments(src): the shipped renderer and its suites `DynamicForm.qml` carried 30 citations, most of them attached to a sentence in the past tense about what the renderer once did with a schema shape. Every one of those is better as the present-tense statement of what the branch is for -- "a renderer that collapses the `oneOf` to its first non-null branch draws a plain TextField over a three-value set" says the same thing to someone reading the branch, and stays true. The locale block keeps every measured figure: 77 of 711 locales with a non-ASCII negative sign, 76 with a non-ASCII zeroDigit, 0 with a multi-unit separator against 54 for each sign. Those are the facts that make the code's shape correct and they are why that comment is long. `tst_i18n.qml`'s section headers were ticket numbers; they are now what the section is about. Catch2 tags in `tests/` (`[morph497]` and friends) are identifiers rather than comments and are left alone, as is the one `test_a_nullable_ref_member_keeps_morph189s_numeric_encoding` function name -- renaming either is a code change, not a comment change. "UAX #44" survives the citation grep and is a Unicode standard reference, not a tracker link. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- src/qt/forms/CMakeLists.txt | 8 +- src/qt/forms/qml/CollectionView.qml | 2 +- src/qt/forms/qml/DynamicForm.qml | 150 +++++++++--------- src/qt/forms/qml/JsonExact.js | 7 +- src/qt/forms/tests/data/instance_bounds.json | 2 +- src/qt/forms/tests/data/rule_corpus.json | 2 +- .../tests/test_forms_controller_core.cpp | 2 +- .../tests/tst_DynamicFormBooleanAndAnyOf.qml | 4 +- .../tst_DynamicFormChoicelessController.qml | 2 +- .../forms/tests/tst_DynamicFormEnumChoice.qml | 24 +-- .../tests/tst_DynamicFormExactBounds.qml | 20 +-- .../tests/tst_DynamicFormFieldBounds.qml | 2 +- .../tests/tst_DynamicFormInstanceBounds.qml | 8 +- .../tests/tst_DynamicFormNestedAggregate.qml | 15 +- .../tests/tst_DynamicFormRuleAgreement.qml | 14 +- .../forms/tests/tst_DynamicFormRuleCorpus.qml | 11 +- .../tests/tst_DynamicFormSchemaAsVariant.qml | 33 ++-- .../tests/tst_DynamicFormUnknownRuleKind.qml | 12 +- src/qt/forms/tests/tst_LargeIdPrecision.qml | 18 +-- src/qt/forms/tests/tst_i18n.qml | 82 +++++----- src/qt/forms/tests/tst_main.cpp | 4 +- src/qt/qt_websocket_backend.cpp | 29 ++-- src/qt/qt_websocket_server.cpp | 4 +- 23 files changed, 220 insertions(+), 235 deletions(-) diff --git a/src/qt/forms/CMakeLists.txt b/src/qt/forms/CMakeLists.txt index f301eabe..c5f9963a 100644 --- a/src/qt/forms/CMakeLists.txt +++ b/src/qt/forms/CMakeLists.txt @@ -35,9 +35,9 @@ target_link_libraries(morph_forms_module PUBLIC morph::morph Qt6::Quick Qt6::Qml target_compile_features(morph_forms_module PUBLIC cxx_std_23) # The shipped renderer's own compiled code (I18nCatalog) and the two suites -# below were never sanitizer-instrumented (morph#542): both suites are ctest -# cases, so a sanitizer preset with -DMORPH_BUILD_FORMS_QML=ON ran them and -# learned nothing. Same static-library caveat as morph_qt_impl in the root +# below need sanitizer instrumentation of their own: both suites are ctest +# cases, so without it a sanitizer preset with -DMORPH_BUILD_FORMS_QML=ON runs +# them and learns nothing. Same static-library caveat as morph_qt_impl in the root # CMakeLists.txt -- every consumer here (the QML plugin, both test # executables) is instrumented on the same preset. if(DEFINED AF_SANITIZER) @@ -64,7 +64,7 @@ if(MORPH_BUILD_TESTS AND NOT EMSCRIPTEN) apply_sanitizers(morph_forms_qml_tests ${AF_SANITIZER}) endif() - # The shared x-rules corpus (morph#176) has two readers: this suite and + # The shared x-rules corpus has two readers: this suite and # tests/test_forms_rule_corpus.cpp. Both are pointed at the one file from # CMake so neither can quietly grow its own copy. tst_main.cpp reads it and # publishes it to the QML engine -- QML's own XMLHttpRequest refuses a diff --git a/src/qt/forms/qml/CollectionView.qml b/src/qt/forms/qml/CollectionView.qml index b947d49c..d11abbfb 100644 --- a/src/qt/forms/qml/CollectionView.qml +++ b/src/qt/forms/qml/CollectionView.qml @@ -120,7 +120,7 @@ Frame { // as exact digits (see JsonExact.js) and must be emitted verbatim. // Re-serialising it as a double rounds it, and because doubles round // to even in that range neighbouring ids collapse -- so Delete on one - // row built a body naming another (morph#191). + // row would build a body naming another. parts.push(JSON.stringify(actionField) + ":" + JsonExact.literal(row[bind[actionField]])) return "{" + parts.join(",") + "}" } diff --git a/src/qt/forms/qml/DynamicForm.qml b/src/qt/forms/qml/DynamicForm.qml index f5c4b4f2..d6e854c9 100644 --- a/src/qt/forms/qml/DynamicForm.qml +++ b/src/qt/forms/qml/DynamicForm.qml @@ -41,7 +41,7 @@ Frame { property var schema property var controller - // The schema as ordinary JSON data, however it was supplied (morph#388). + // The schema as ordinary JSON data, however it was supplied. // **Every read of the schema below goes through this, never through // `schema` itself.** // @@ -59,11 +59,10 @@ Frame { // strong id under `$defs`) was wrapped rather than unpacked, leaving // `isInteger` false, so the id was submitted as a quoted JSON // *string* and the server answered parse_number_failure; - // - the `anyOf`-over-`$ref` collapse (morph#189) never ran, so a - // nullable `$ref` member regressed to that same encoding -- #189's - // own fix, going inert on this path; - // - the closed-set recognition (morph#386) never ran, so an `enum` - // drew a free-text box over what the schema states is a closed set. + // - the `anyOf`-over-`$ref` collapse never runs, so a nullable `$ref` + // member falls back to that same encoding; + // - the closed-set recognition never runs, so an `enum` draws a + // free-text box over what the schema states is a closed set. // // Normalising once, here, is what stops that being a standing trap for // the next `Array.isArray` anyone writes against schema data. @@ -183,7 +182,7 @@ Frame { return typeof value === "number" && isFinite(value) ? value : undefined } - // Whether `value` breaks a declared `multipleOf` (morph#310). An absent or + // Whether `value` breaks a declared `multipleOf`. An absent or // non-positive step is no constraint at all, matching JSON Schema, which // requires `multipleOf` to be strictly positive. // @@ -204,7 +203,7 @@ Frame { // Three-way compare of two integers held as decimal strings: -1, 0, 1. // Needed because a JS number cannot hold an int64 bound exactly, so the - // comparison has to happen on digits (morph#213). Inputs are already + // comparison has to happen on digits. Inputs are already // /^-?\d+$/-validated by the caller. function compareIntText(left, right) { const leftNeg = left.charAt(0) === "-" @@ -243,7 +242,7 @@ Frame { // "type" key**. Resolving only the top-level $ref left every kind flag // below false, so the value fell through to the plain-text encoding and // went out as a quoted JSON *string* that the server then rejected with - // parse_number_failure (morph#189). Resolve through the non-null branch + // parse_number_failure. Resolve through the non-null branch // so the field is typed by T. `oneOf` is handled the same way — glaze // emits it for every `glz::enumerate`d `enum class`, and a // hand-written or evolved schema may use it for nullability too. @@ -262,7 +261,7 @@ Frame { // A closed set of alternatives is not the nullability shape // this collapse exists for: its branches differ in *value*, // so the first one's `const` is not the field's own and must - // not be left masquerading as it (morph#386). The set itself + // not be left masquerading as it. The set itself // survives via enumChoices() below, which reads the branches // rather than this collapsed node. delete merged["const"] @@ -292,7 +291,7 @@ Frame { // a hand-written or evolved schema may: `{"enum": ["a", "b"]}`. // // This is distinguishable from the nullable-`$ref` shape resolveProp - // collapses (morph#189), whose branches carry no `const` at all: **one** + // collapses, whose branches carry no `const` at all: **one** // branch without a `const` and the property is not a closed set, so the // whole thing falls back rather than offering a partial list. // @@ -384,7 +383,7 @@ Frame { const types = Array.isArray(p.type) ? p.type : (p.type === undefined ? [] : [p.type]) const dp = opt(raw["x-decimalPlaces"], p["x-decimalPlaces"]) const optionsAction = opt(raw["x-optionsAction"], p["x-optionsAction"]) - // A closed set stated by the schema itself (morph#386). Read + // A closed set stated by the schema itself. Read // from `raw`, not the collapsed `p`: resolveProp keeps only // one branch, and the set is the point. A field that also // declares x-optionsAction is a server-fetched Choice and @@ -470,7 +469,7 @@ Frame { required: required.indexOf(name) !== -1, // `resolveRef` merges the property node *over* the `$def` // it points at, so these three read a per-field bound - // declared through `FieldMeta` (morph#310) as readily as + // declared through `FieldMeta` as readily as // one glaze stamped on the shared type definition -- which // is what makes a bound on one `Quantity` member leave a // sibling of the same type alone. @@ -487,16 +486,16 @@ Frame { // `JSON.parse(controller.schemasJson)`), so an int64 bound // is already rounded by the time it gets here -- INT64_MAX // arrives as 9223372036854775808. These strings are not - // (morph#213). Undefined for any bound a double holds + // Undefined for any bound a double holds // exactly, which is the overwhelmingly common case. exactMinimum: p["x-exactMinimum"], exactMaximum: p["x-exactMaximum"], // Per-*instance* bounds a model wrote into the served - // schema from data (morph::forms::InstanceConstraints; - // morph#164). `{num,den,dp}` Rational nodes, never plain + // schema from data (morph::forms::InstanceConstraints). + // `{num,den,dp}` Rational nodes, never plain // numbers, and never emitted by schemaJson() itself. - // Without these the renderer honoured only the compiled - // `minimum`/`maximum` and an instance's own range was + // Without these the renderer honours only the compiled + // `minimum`/`maximum` and an instance's own range is // decorative -- exactly the "two values for one concept, // and the renderer believes the compiled one" outcome the // decoration seam exists to remove. @@ -624,7 +623,7 @@ Frame { // CheckBox writes those), but `equals` against a `bool` emits a JSON // *boolean* literal. Comparing the two as text made `"true" === true` // false, so a requiredWhen keyed on a boolean never fired on the - // client while the compiled evaluator fired it (morph#176). + // client while the compiled evaluator fires it. if (meta && meta.isBoolean) return text === "true" if (meta && (meta.isQuantity || meta.isInteger)) @@ -638,11 +637,11 @@ Frame { // // **Three-valued.** `true` / `false` / `undefined`, where `undefined` is // "this renderer cannot evaluate this node" -- an unrecognised `kind`. - // That is a distinct answer from `false`, and collapsing the two is what - // made the shipped renderer contradict every other client of the same - // spec sentence (morph#176): a `not` wrapping an unknown child came out - // *true*, so a requiredWhen keyed on it started demanding a field for a - // reason the renderer had just admitted it could not judge. + // That is a distinct answer from `false`, and collapsing the two makes + // this renderer contradict every other client of the same spec sentence: + // a `not` wrapping an unknown child comes out *true*, so a requiredWhen + // keyed on it demands a field for a reason the renderer has just admitted + // it cannot judge. // // `undefined` propagates. `and` is `false` if any child is false, and // `undefined` if none is false but some is unevaluable. `or` is `true` if @@ -663,8 +662,7 @@ Frame { // An integral literal beyond 2^53 arrives here already rounded by // JSON.parse, so comparing it as a number collapses values the // schema kept distinct. `valueText` carries the exact digits when - // the emitter judged the number unsafe; compare on digits then - // (morph#176). + // the emitter judged the number unsafe; compare on digits then. if (cond.valueText !== undefined) { const text = (opt(fieldValues[names[0]], "")).trim() if (!/^-?\d+$/.test(text)) @@ -807,26 +805,25 @@ Frame { // The payload's exact digit routines below stay entirely locale-free — // this is the one control-edge conversion step, applied once per entry. - // Grouping is *validated*, not stripped (morph#574). A group separator is + // Grouping is *validated*, not stripped. A group separator is // only dropped where one can legally be -- preceded by one to three digits, // followed by exactly three more, never after the decimal separator. - // Stripping it unconditionally, which both this function and its C++ twin - // used to do, turns a de-DE user's US-style "1.5" into 15: a valid number, - // ten times too large, that nothing downstream can recognise as wrong. - // Verified against the C++ side on the same inputs before and after; the - // two edges agreed on every wrong answer and now agree on every rejection. - // The negative sign is matched as a whole string, not as one code unit - // (morph#583). 77 of the 711 locales Qt 6.11.2 knows spell it as something + // Stripping it unconditionally turns a de-DE user's US-style "1.5" into 15: + // a valid number, ten times too large, that nothing downstream can + // recognise as wrong. The C++ edge validates on the same rule, and the two + // are checked against each other on the same inputs. + // The negative sign is matched as a whole string, not as one code unit. + // 77 of the 711 locales Qt 6.11.2 knows spell it as something // other than a bare ASCII "-": 23 use U+2212, and 54 prefix it with a bidi // control mark (U+061C, U+200E, U+200F), making it two or three code units // -- ar_DZ does so even though its sign *is* the ordinary hyphen. `ch === - // "-"` matched none of them, and formatCanonicalNumber emitted a sign this + // "-"` matches none of them, and formatCanonicalNumber would emit a sign this // function then rejected. A bare "-" stays accepted alongside the locale's // own spelling: U+2212 and the bidi marks are on no keyboard, so matching // only the locale spelling would leave those users no way to type a // negative number at all. An omitted or empty negativeSign reads as "-", // not as "no sign" -- there is no locale without one. - // A leading positive sign is accepted and *dropped* (morph#596): canonical + // A leading positive sign is accepted and *dropped*: canonical // text is -?[0-9]+(\.[0-9]+)?, which has no "+" in it, so "+5" yields "5". // 54 of the 711 locales spell the positive sign with a bidi control mark // before the "+" (U+061C, U+200E, U+200F), and unlike the negative side @@ -836,7 +833,7 @@ Frame { // emits one: a positive displays unsigned in every locale, and emitting the // sign would turn every positive number in every form from "5" into "+5". // The pair is therefore deliberately not inverse across a positive sign. - // The digits are locale data too (morph#591), carried as a *base*: a + // The digits are locale data too, carried as a *base*: a // Unicode decimal digit set is ten contiguous code points by definition // (UAX #44), so one zeroDigit is enough and a ten-element table is not // needed. 76 of the 711 locales Qt 6.11.2 knows report a zeroDigit other @@ -844,31 +841,26 @@ Frame { // (U+11136 Chakma, U+1E950 Adlam), which is why this scans code *points* // via codePointAt and steps two units for one digit when it has to. Entry // accepts a digit in [zeroDigit, zeroDigit+9] or in ["0","9"]; display - // emits only the locale's. That asymmetry is the morph#596 rule applied to - // digits: the locale's own digits are on the user's keyboard only if their + // emits only the locale's. That asymmetry is the positive-sign rule applied + // to digits: the locale's own digits are on the user's keyboard only if their // keyboard has them, and accepting an extra spelling cannot change a value // because the canonical output always spells digits in ASCII. Mixing the // two families in one entry is malformed -- see below. - // All five facts now travel as one object rather than as five positional - // arguments, mirroring the C++ NumericLocale aggregate: on that side the - // row of interchangeable string_views needed a clang-tidy suppression for - // bugprone-easily-swappable-parameters, and a sixth would have made the - // argument for it weaker rather than stronger. Here the gain is the same - // one a reader gets -- a call names each fact -- and it keeps the two - // mirrors structurally identical, which is the property morph#599 and - // morph#591 were both about. - // The two *separators* are matched as whole strings for the same reason - // (morph#599), and that this was not already true was the mixed idiom - // morph#583 and morph#596 left behind: they converted the signs to - // `text.startsWith(sign, i)` and left the separators on `ch === sep`, one - // UTF-16 code unit, a few lines apart with nothing saying why. Unlike the - // signs, no locale reaches this: measured over the 711 locales Qt 6.11.2 - // reports, *every* decimalPoint and *every* groupSeparator is exactly one - // code unit (0 multi-unit, against 54 for each sign). So this changes what - // no user could reach, and fixes what every reader of these thirty lines - // could: docs/spec/forms/forms.md, "Both edges, or neither" -- the C++ edge - // has always matched separators whole (`rest.starts_with(...)`), and a - // divergence between the two is a divergence in what the product accepts. + // All five facts travel as one object rather than as five positional + // arguments, mirroring the C++ NumericLocale aggregate: on that side a row + // of interchangeable string_views would need a clang-tidy suppression for + // bugprone-easily-swappable-parameters. Here the gain is the one a reader + // gets -- a call names each fact -- and it keeps the two mirrors + // structurally identical. + // The two *separators* are matched as whole strings for the same reason the + // signs are, and uniformly with them: `text.startsWith(sep, i)`, not + // `ch === sep` over one UTF-16 code unit. No locale reaches the difference + // -- measured over the 711 locales Qt 6.11.2 reports, *every* decimalPoint + // and *every* groupSeparator is exactly one code unit, against 54 + // multi-unit spellings for each sign -- so this is for the reader, and for + // docs/spec/forms/forms.md, "Both edges, or neither": the C++ edge matches + // separators whole (`rest.starts_with(...)`), and a divergence between the + // two is a divergence in what the product accepts. // The digit-set base of a NumericLocale-shaped object: the code point of // its zeroDigit, or ASCII "0" when it is absent or empty. Empty reads as // the default for the reason an empty negativeSign does -- there is no @@ -938,7 +930,7 @@ Frame { sawDecimal = true canonical += "." // The decimal point is output, so a sign straight after it is - // not leading (morph#497). + // not leading. sawAnyOutput = true i += decimalSeparator.length - 1 // the loop's ++i consumes the last unit continue @@ -1011,12 +1003,12 @@ Frame { return (cp >= 0x30 && cp <= 0x39) ? String.fromCodePoint(base + (cp - 0x30)) : ch } - // The locale's positiveSign is deliberately not read here (morph#596): a - // positive number displays unsigned in every locale, so the entry edge above - // accepts a leading "+" that this edge never produces. The digits, by - // contrast, *are* emitted in the locale's set (morph#591) -- this edge had - // to move with the entry edge or the pair would no longer be inverse, which - // is the round trip docs/spec/forms/forms.md requires. + // The locale's positiveSign is deliberately not read here: a positive + // number displays unsigned in every locale, so the entry edge above accepts + // a leading "+" that this edge never produces. The digits, by contrast, + // *are* emitted in the locale's set -- this edge has to match the entry + // edge or the pair is not inverse, which is the round trip + // docs/spec/forms/forms.md requires. function formatCanonicalNumber(text, locale) { const loc = locale ? locale : {} const decimalSeparator = loc.decimalSeparator !== undefined ? loc.decimalSeparator : "." @@ -1177,8 +1169,8 @@ Frame { // Also already a JSON literal — but here the whole set is in the // schema, so membership is decidable *on the client*, and a value // outside it is invalid rather than merely "the server will say - // no". Without this the form reported ready for role="Emperor" - // and assembled a body for it (morph#386), which is the opposite + // no". Without this the form reports ready for role="Emperor" + // and assembles a body for it, which is the opposite // of what a submit gate is for. Same reason isBoolean refuses // anything but true/false. A server-fetched Choice below is // deliberately not checked this way: its option list is a @@ -1220,7 +1212,7 @@ Frame { return null if (f.maximum !== undefined && value > f.maximum) return null - // A decorated schema's per-instance range (morph#164). Narrows + // A decorated schema's per-instance range. Narrows // the compiled bound; it never widens it, because both are // checked. Quantity fields only, matching what // InstanceConstraints::checkAction checks server-side -- a @@ -1243,7 +1235,7 @@ Frame { // Prefer the exact string bound when the schema carries one: a // double-valued bound rounds at 2^53, and comparing INT64_MAX + 1 // against a maximum rounded *up* to 9223372036854775808 judges it - // "not greater" and lets it through the gate (morph#213). + // "not greater" and lets it through the gate. const value = parseInt(text) if (f.exactMinimum !== undefined) { if (compareIntText(normalised, f.exactMinimum) < 0) @@ -1365,7 +1357,7 @@ Frame { const name = form.fields[i].name const entry = form.findControl(form, "field_" + name) if (entry) { - // An enum's combo box claims this objectName (morph#386) + // An enum's combo box claims this objectName // and carries no writable `text` -- "no selection" is // currentIndex -1, the state it is created in. if (form.fields[i].isEnum) @@ -1468,8 +1460,8 @@ Frame { // `optionsReceived` is not. It exists only on a controller that serves a // morph::forms::Choice field, and a controller that serves none declares // no stub for it -- the sanctioned shape (bookmarks' and pastebin's forms - // controllers both document why). Unaccommodated, that shape made the form - // warn once per instance about the handler below (morph#387). + // controllers both document why). Unaccommodated, that shape makes the form + // warn once per instance about the handler below. // // The accommodation is the gated target, not `ignoreUnknownSignals`. A // controller without the signal is never connected to, so there is nothing @@ -1489,7 +1481,7 @@ Frame { // Exact-int aware: an option id above 2^53 is rounded by a plain // JSON.parse, and re-stringifying the rounded number selects a // different row -- or, for a dense id range, makes two options - // indistinguishable from each other (morph#190). + // indistinguishable from each other. try { parsed = JsonExact.parse(payload) } catch (ignored) { return } for (let i = 0; i < form.fields.length; ++i) { const f = form.fields[i] @@ -1581,7 +1573,7 @@ Frame { // One combo box for both closed sets: the server-fetched // Choice (x-optionsAction) and the schema-stated enum - // (a `oneOf` of `const`s, or a bare `enum`; morph#386). They + // (a `oneOf` of `const`s, or a bare `enum`). They // differ only in where the rows come from -- an enum's are // already in the schema, so it never fetches -- and the rows // have the same {label, valueJson} shape either way. @@ -1761,10 +1753,10 @@ Frame { } // "boolean" — a CheckBox. The plain TextField's fall-through - // encoded the typed text as a JSON *string* ({"flag":"true"}), - // and applied no validation at all, so "banana" was accepted - // and sent; glaze rejected both with expected_true_or_false - // (morph#189). A CheckBox can only produce the two valid + // would encode the typed text as a JSON *string* + // ({"flag":"true"}), and apply no validation at all, so + // "banana" would be accepted and sent; glaze rejects both with + // expected_true_or_false. A CheckBox can only produce the two valid // spellings. Reuses the plain TextField's field_ objectName — // the two are mutually exclusive per field (isBoolean), so // exactly one claims it. diff --git a/src/qt/forms/qml/JsonExact.js b/src/qt/forms/qml/JsonExact.js index b4ab3840..384257ae 100644 --- a/src/qt/forms/qml/JsonExact.js +++ b/src/qt/forms/qml/JsonExact.js @@ -3,15 +3,14 @@ // JSON parsing that does not silently round an integer a double cannot hold. // // Every value the renderer receives from an app -- query results, choice option -// rows -- arrives as JSON text and used to go through plain `JSON.parse`. +// rows -- arrives as JSON text, and plain `JSON.parse` cannot carry it. // JavaScript numbers are IEEE-754 doubles, so an integer above 2^53 does not // survive: it is rounded on the way in, and re-serialising the rounded number // emits a *different* id than the app sent. Because doubles round to even in // that range, neighbouring ids collapse onto the same value, so two distinct -// rows can become indistinguishable -- deleting one deletes the other -// (morph#190, morph#191). +// rows become indistinguishable -- deleting one deletes the other. // -// The fix keeps the digits. parse() finds integer literals a double cannot +// This module keeps the digits. parse() finds integer literals a double cannot // represent exactly and hands them back as exact-int wrappers carrying the // original text; literal() re-emits those digits verbatim. Values a double // *does* hold exactly -- the overwhelmingly common case -- stay ordinary diff --git a/src/qt/forms/tests/data/instance_bounds.json b/src/qt/forms/tests/data/instance_bounds.json index ff8060b1..41aeda79 100644 --- a/src/qt/forms/tests/data/instance_bounds.json +++ b/src/qt/forms/tests/data/instance_bounds.json @@ -1,6 +1,6 @@ { "readme": [ - "The per-instance constraints corpus (morph#164). ONE file, TWO readers:", + "The per-instance constraints corpus. ONE file, TWO readers:", " tests/test_forms_instance_constraints.cpp -- InstanceConstraints::checkValue", " src/qt/forms/tests/tst_DynamicFormInstanceBounds.qml -- a real DynamicForm", "", diff --git a/src/qt/forms/tests/data/rule_corpus.json b/src/qt/forms/tests/data/rule_corpus.json index 876b515f..1e90202a 100644 --- a/src/qt/forms/tests/data/rule_corpus.json +++ b/src/qt/forms/tests/data/rule_corpus.json @@ -1,6 +1,6 @@ { "readme": [ - "The shared x-rules corpus (morph#176). ONE file, TWO readers:", + "The shared x-rules corpus. ONE file, TWO readers:", " tests/test_forms_rule_corpus.cpp -- morph::forms::allRulesSatisfied", " src/qt/forms/tests/tst_DynamicFormRuleCorpus.qml -- a real DynamicForm", "Neither owns the cases, so the two evaluators cannot be pinned to", diff --git a/src/qt/forms/tests/test_forms_controller_core.cpp b/src/qt/forms/tests/test_forms_controller_core.cpp index 1b26c090..d149bca2 100644 --- a/src/qt/forms/tests/test_forms_controller_core.cpp +++ b/src/qt/forms/tests/test_forms_controller_core.cpp @@ -146,7 +146,7 @@ TEST_CASE("morph::qt::forms::FormsControllerCore forwards fetchOptions' body, no TEST_CASE("morph::qt::forms::FormsControllerCore composes over a caller-supplied Bridge/executor", "[forms_controller_core]") { - // Issue #57: FormsControllerCore must be usable against a Bridge the + // FormsControllerCore must be usable against a Bridge the // caller already owns (e.g. one already switched to a Remote/Socket // backend, or shared across multiple presenters) instead of always // building and owning its own private, always-local Bridge. diff --git a/src/qt/forms/tests/tst_DynamicFormBooleanAndAnyOf.qml b/src/qt/forms/tests/tst_DynamicFormBooleanAndAnyOf.qml index c0ecf530..5df2e452 100644 --- a/src/qt/forms/tests/tst_DynamicFormBooleanAndAnyOf.qml +++ b/src/qt/forms/tests/tst_DynamicFormBooleanAndAnyOf.qml @@ -1,7 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 // -// Covers the two field shapes DynamicForm used to encode with the wrong JSON -// type, producing payloads the server rejected (morph#189): +// Covers the two field shapes a renderer most easily encodes with the wrong +// JSON type, producing payloads the server rejects: // // 1. {"type": "boolean"} fell through to the plain TextField, which applied // no validation and emitted the typed text as a JSON *string* -- diff --git a/src/qt/forms/tests/tst_DynamicFormChoicelessController.qml b/src/qt/forms/tests/tst_DynamicFormChoicelessController.qml index 87e29c91..f94fc8a3 100644 --- a/src/qt/forms/tests/tst_DynamicFormChoicelessController.qml +++ b/src/qt/forms/tests/tst_DynamicFormChoicelessController.qml @@ -1,7 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 // // A controller that serves no `morph::forms::Choice` field must load a form -// without the engine warning about it (morph#387). +// without the engine warning about it. // // `optionsReceived` only exists on a controller that serves a Choice. A // controller that serves none does not declare it, and deliberately does not: diff --git a/src/qt/forms/tests/tst_DynamicFormEnumChoice.qml b/src/qt/forms/tests/tst_DynamicFormEnumChoice.qml index 419c16cc..dbe10800 100644 --- a/src/qt/forms/tests/tst_DynamicFormEnumChoice.qml +++ b/src/qt/forms/tests/tst_DynamicFormEnumChoice.qml @@ -2,11 +2,11 @@ // // A C++ `enum class` member is fully described by the schema: glaze emits it // as a closed `oneOf` of `const` alternatives, each with its own `title`. -// DynamicForm threw all of it away (morph#386) -- resolveProp collapsed the -// `oneOf` to its first non-null branch (the nullable-$ref path morph#189 -// added, where the branches differ only in nullability), and no field flag -// ever looked at `const`. The result was a plain TextField for a three-value -// set, and a submit gate that reported `ready` for role = "Emperor". +// A renderer that collapses the `oneOf` to its first non-null branch -- the +// nullable-$ref path, where the branches differ only in nullability -- and +// never looks at `const` throws all of that away: a plain TextField for a +// three-value set, and a submit gate that reports `ready` for role = +// "Emperor". These cases pin the recognition that prevents it. // // Every schema below is pasted **verbatim** from // `morph::forms::schemaJson()` for a shipped action, so a change in what @@ -100,8 +100,8 @@ TestCase { // `partial` -- a `oneOf` in which one branch pins no value. Not a // closed set; offering a two-of-three list would be worse // than the text field. - // `optI64` -- the nullable-$ref shape morph#189 added the collapse - // for. It carries no `const` and must keep collapsing. + // `optI64` -- the nullable-$ref shape the collapse exists for. It + // carries no `const` and must keep collapsing. property var handWrittenSchema: ({ "$defs": { "int64_t": { "type": "integer" } }, "properties": { @@ -216,8 +216,8 @@ TestCase { } function test_the_nullable_ref_anyOf_still_collapses_to_its_typed_branch() { - // morph#189's shape. Its branches carry no `const`, so it is not a - // closed set and must still be typed by T rather than drawn as a + // The nullable-$ref shape. Its branches carry no `const`, so it is not + // a closed set and must still be typed by T rather than drawn as a // picker over nothing. var form = createTemporaryObject(handWrittenForm, testCase) var optI64 = meta(form, "optI64") @@ -292,9 +292,9 @@ TestCase { var form = createTemporaryObject(roleForm, testCase) findChild(form, "field_projectId").text = "1" findChild(form, "field_principal").text = "bob" - // The exact case morph#386 measured: before the fix `ready` was true - // and a body was assembled for it, so the client gate said the - // opposite of what it exists to say. + // A value outside the closed set. Without the recognition above, + // `ready` is true and a body is assembled for it, so the client gate + // says the opposite of what it exists to say. form.setFieldValue("role", '"Emperor"') compare(form.ready, false) compare(form.previewLine, "") diff --git a/src/qt/forms/tests/tst_DynamicFormExactBounds.qml b/src/qt/forms/tests/tst_DynamicFormExactBounds.qml index d655d8d9..b0b86041 100644 --- a/src/qt/forms/tests/tst_DynamicFormExactBounds.qml +++ b/src/qt/forms/tests/tst_DynamicFormExactBounds.qml @@ -1,6 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // -// The client-side integer bounds gate at INT64 extremes (morph#213). +// The client-side integer bounds gate at INT64 extremes. // // `minimum`/`maximum` reach this renderer through // `JSON.parse(controller.schemasJson)`, which every shipped app does, so an @@ -53,17 +53,17 @@ TestCase { required: ["id"] }) - // The same field with no exact companions: the pre-#213 shape, kept so the - // numeric fallback path stays covered. + // The same field with no exact companions, so the numeric fallback path + // stays covered. property var smallBoundSchema: ({ properties: { n: { type: "integer", minimum: -10, maximum: 10, "x-order": 0 } }, required: ["n"] }) - // The anyOf shape: a bare std::optional. Before morph#189 this - // had no resolved type at all, so no bounds applied and the gate never ran. - // Now that resolveProp follows the non-null anyOf branch, the field inherits - // $defs/int64_t's bounds -- including the exact companions (morph#213). + // The anyOf shape: a bare std::optional. Without a resolved + // type no bounds apply and the gate never runs; because resolveProp follows + // the non-null anyOf branch, the field inherits $defs/int64_t's bounds -- + // including the exact companions. property var anyOfI64Schema: ({ "$defs": { "int64_t": { @@ -174,9 +174,9 @@ TestCase { function test_anyOf_field_inherits_the_exact_bounds_and_rejects_past_them() { var form = createTemporaryObject(anyOfI64Form, testCase) - // Measured on morph#189's branch before this fix: an anyOf int64 field - // admitted INT64_MAX + 1, because the bound it compared against had been - // rounded up by JSON.parse to exactly that value. + // Without the exact companions, an anyOf int64 field admits + // INT64_MAX + 1, because the bound it compares against has been rounded + // up by JSON.parse to exactly that value. typeInto(form, "field_optId", "9223372036854775808") compare(form.ready, false) } diff --git a/src/qt/forms/tests/tst_DynamicFormFieldBounds.qml b/src/qt/forms/tests/tst_DynamicFormFieldBounds.qml index fc8fcca6..134de0ed 100644 --- a/src/qt/forms/tests/tst_DynamicFormFieldBounds.qml +++ b/src/qt/forms/tests/tst_DynamicFormFieldBounds.qml @@ -1,6 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // -// The renderer half of the per-field scalar bounds (morph#310). +// The renderer half of the per-field scalar bounds. // // `FieldMeta::minimum`/`::maximum`/`::multipleOf` let an action declare a // bound the `formRules` vocabulary cannot express -- every comparison node diff --git a/src/qt/forms/tests/tst_DynamicFormInstanceBounds.qml b/src/qt/forms/tests/tst_DynamicFormInstanceBounds.qml index d824798a..ccb94aa9 100644 --- a/src/qt/forms/tests/tst_DynamicFormInstanceBounds.qml +++ b/src/qt/forms/tests/tst_DynamicFormInstanceBounds.qml @@ -1,6 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // -// The renderer half of the per-instance constraints corpus (morph#164). +// The renderer half of the per-instance constraints corpus. // // `schemaJson()` is a pure function of the compiled action type, so a form // whose *definition* is data -- a versioned analysis catalogue whose version 1 @@ -40,9 +40,9 @@ TestCase { // Bound into the component rather than passed through // createTemporaryObject's initial properties. Both paths render the same - // form since morph#388 (`DynamicForm` re-reads the schema as JSON at the - // property, so the QVariantMap conversion no longer changes what it sees); - // binding stays because it is what every shipped app does, and because + // form (`DynamicForm` re-reads the schema as JSON at the property, so the + // QVariantMap conversion cannot change what it sees); + // binding is used because it is what every shipped app does, and because // `pendingSchema` is re-assigned per corpus row and the binding re-renders // on its own. property var pendingSchema: ({}) diff --git a/src/qt/forms/tests/tst_DynamicFormNestedAggregate.qml b/src/qt/forms/tests/tst_DynamicFormNestedAggregate.qml index 10673910..457c0873 100644 --- a/src/qt/forms/tests/tst_DynamicFormNestedAggregate.qml +++ b/src/qt/forms/tests/tst_DynamicFormNestedAggregate.qml @@ -1,13 +1,12 @@ // SPDX-License-Identifier: Apache-2.0 // // What DynamicForm does with a nested-aggregate member, cyclic or not -// (morph#727; docs/spec/forms/forms.md, "What DynamicForm does with a nested -// aggregate"). +// (docs/spec/forms/forms.md, "What DynamicForm does with a nested aggregate"). // -// morph#703 made `schemaJson()` emit a finite, correctly annotated schema -// for a self-referential domain type, by way of a `$ref` back into `$defs`. -// It did not say what the shipped renderer should draw for one, and the spec -// said only that morph "does not promise to render the form". This suite is +// `schemaJson()` emits a finite, correctly annotated schema for a +// self-referential domain type, by way of a `$ref` back into `$defs`. +// That says nothing about what the shipped renderer draws for one, and the +// spec says only that morph "does not promise to render the form". This suite is // the measurement that replaced that non-promise with a stated contract, and // it pins every part of it: // @@ -24,8 +23,8 @@ // member submits as a JSON *string*, an array-of-objects member as an // array of strings, and `ready` is true for both. // -// (4) is the part worth arguing about, and the argument is morph#759, not -// this file. This suite states today's behaviour so that a change to it is +// (4) is the part worth arguing about, and this file is not the place to +// argue it. This suite states the current behaviour so that a change to it is // visible as a failing test rather than as a silent difference. import QtQuick diff --git a/src/qt/forms/tests/tst_DynamicFormRuleAgreement.qml b/src/qt/forms/tests/tst_DynamicFormRuleAgreement.qml index 03370e96..ded0fd99 100644 --- a/src/qt/forms/tests/tst_DynamicFormRuleAgreement.qml +++ b/src/qt/forms/tests/tst_DynamicFormRuleAgreement.qml @@ -1,8 +1,8 @@ // SPDX-License-Identifier: Apache-2.0 // // `x-rules` is evaluated twice — once compiled (`morph::forms::allRulesSatisfied`) -// and once in JavaScript here — and nothing pinned the two to each other -// (morph#176). These cases drive the *verbatim* `schemaJson()` output of a +// and once in JavaScript here — so something has to pin the two to each other. +// These cases drive the *verbatim* `schemaJson()` output of a // real action through the renderer and assert the verdict the compiled // evaluator reaches for the same field state. // @@ -57,8 +57,8 @@ TestCase { flag.checked = true flag.toggled() // Compiled: flag == true, so `reason` is required and unset -> not satisfied. - // Before morph#176 the client compared "true" === true and never fired, - // so it reported ready and submitted a body the server rejects. + // A client comparing "true" === true would never fire this rule, report + // ready, and submit a body the server rejects. compare(f.ready, false) findChild(f, "field_reason").text = "because" @@ -80,9 +80,9 @@ TestCase { var f = createTemporaryObject(form, testCase) // 9007199254740992 is one below the literal 9007199254740993. The // compiled evaluator says the condition is false, so `reason` is not - // required. JSON.parse collapses both to the same double, so before - // morph#176 the client believed the condition held and blocked a - // submission the server would have accepted. + // required. JSON.parse collapses both to the same double, so a client + // that read the literal through it would believe the condition held and + // block a submission the server would have accepted. findChild(f, "field_id").text = "9007199254740992" compare(f.ready, true) } diff --git a/src/qt/forms/tests/tst_DynamicFormRuleCorpus.qml b/src/qt/forms/tests/tst_DynamicFormRuleCorpus.qml index 2aff18bb..e16758b7 100644 --- a/src/qt/forms/tests/tst_DynamicFormRuleCorpus.qml +++ b/src/qt/forms/tests/tst_DynamicFormRuleCorpus.qml @@ -1,6 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // -// The renderer half of the shared `x-rules` corpus (morph#176). +// The renderer half of the shared `x-rules` corpus. // // `x-rules` is evaluated twice -- compiled by `morph::forms::allRulesSatisfied` // and again in JavaScript by DynamicForm.qml -- and nothing structural pinned @@ -41,11 +41,10 @@ TestCase { // The schema the next createTemporaryObject() picks up. A `property var` // assigned a JS value, then *bound* into the component below rather than // handed to createTemporaryObject as an initial-properties entry. That - // distinction used to matter -- the initial-properties path converts the - // object through QVariantMap, and `DynamicForm` typed each field by asking - // `Array.isArray` about the result -- but morph#388 made the renderer - // re-read the schema as JSON at the property, so both paths now render the - // same form. Binding stays because it is what every shipped app does, and + // distinction does not matter to what is rendered: `DynamicForm` re-reads + // the schema as JSON at the property, so the initial-properties path's + // conversion through QVariantMap cannot change how a field is typed. + // Binding is used anyway because it is what every shipped app does, and // because re-assigning `pendingSchema` per corpus row re-renders on its own. property var pendingSchema: ({}) diff --git a/src/qt/forms/tests/tst_DynamicFormSchemaAsVariant.qml b/src/qt/forms/tests/tst_DynamicFormSchemaAsVariant.qml index 44262c29..38d73c96 100644 --- a/src/qt/forms/tests/tst_DynamicFormSchemaAsVariant.qml +++ b/src/qt/forms/tests/tst_DynamicFormSchemaAsVariant.qml @@ -1,7 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // -// The same schema, reaching DynamicForm two ways, must render the same form -// (morph#388). +// The same schema, reaching DynamicForm two ways, must render the same form. // // Every shipped app *binds* `schema` declaratively, so the value arrives as a // genuine JS object and every `Array.isArray` in the renderer answers true. A @@ -14,13 +13,12 @@ // question: // // - `{"type": ["integer","null"]}` (what schemaJson emits for a rule-3 -// strong id under `$defs`) wrapped instead of unpacking, so `isInteger` -// came out false and the id went out as a quoted JSON *string* -- the -// parse_number_failure morph#189 already fixed once, through a new door; -// - the `anyOf`-over-`$ref` collapse of morph#189 itself went inert, so a -// nullable `$ref` member regressed to that same pre-#189 encoding; -// - the closed-set recognition of morph#386 (`oneOf` of `const`, and the -// bare `enum` keyword) stopped firing, so an enum drew a free-text box. +// strong id under `$defs`) wraps instead of unpacking, so `isInteger` +// comes out false and the id goes out as a quoted JSON *string*; +// - the `anyOf`-over-`$ref` collapse goes inert, so a nullable `$ref` +// member falls back to that same quoted-string encoding; +// - the closed-set recognition (`oneOf` of `const`, and the bare `enum` +// keyword) stops firing, so an enum draws a free-text box. // // Nothing warned: the form reported `ready` and produced a body the server // refuses. Each case below therefore asserts the two forms against **each @@ -51,7 +49,7 @@ TestCase { // schemaJson() (the same fixture // tst_DynamicFormEnumChoice.qml pins): `projectId` is the array-valued - // `type` of the report, `role` the morph#386 closed set. + // `type` of the report, `role` the closed set. // // The properties are declared `role, principal, projectId` -- deliberately // *not* their sorted order. schemaJson emits them sorted, so a verbatim @@ -81,11 +79,11 @@ TestCase { "required": ["projectId", "principal", "role"] }) - // The remaining array-shaped keys in one schema: the morph#189 + // The remaining array-shaped keys in one schema: the // `anyOf`-over-`$ref` collapse (`optId`), the bare `enum` keyword // (`size`), `type: "array"` (`tags`), plus `required` and `x-layout`, - // which the triage of morph#388 measured as *not* degrading -- kept here - // so a fix that normalises the schema cannot quietly break them. + // measured as *not* degrading on the variant path -- pinned here so a + // change that normalises the schema cannot quietly break them. property var mixedSchema: ({ "$defs": { "int64_t": { "type": "integer", "minimum": -9223372036854775808 } }, "properties": { @@ -220,9 +218,10 @@ TestCase { } function test_a_nullable_ref_member_keeps_morph189s_numeric_encoding() { - // morph#189's own fix reads `anyOf` through Array.isArray, so on this - // path it stopped running: the field lost its type entirely and fell - // back to the quoted-string encoding #189 was written to remove. + // The `anyOf` collapse reads its branches through Array.isArray, which + // answers false on this path unless the schema is re-read as JSON. If + // it does not run, the field loses its type and falls back to the + // quoted-string encoding. var form = viaVariant(variantMixedForm, testCase.mixedSchema) compare(form.fieldByName["optId"].isInteger, true) put(form, "size", 0) @@ -233,7 +232,7 @@ TestCase { } function test_a_closed_set_is_still_drawn_as_a_picker() { - // morph#386's recognition reads `oneOf`/`enum` the same way. + // The closed-set recognition reads `oneOf`/`enum` the same way. var form = viaVariant(variantRoleForm, testCase.roleSchema) compare(form.fieldByName["role"].isEnum, true) compare(form.fieldByName["role"].enumOptions.length, 3) diff --git a/src/qt/forms/tests/tst_DynamicFormUnknownRuleKind.qml b/src/qt/forms/tests/tst_DynamicFormUnknownRuleKind.qml index 56c6a77a..aaa3f1cf 100644 --- a/src/qt/forms/tests/tst_DynamicFormUnknownRuleKind.qml +++ b/src/qt/forms/tests/tst_DynamicFormUnknownRuleKind.qml @@ -1,7 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 // -// The forward-compatibility half of morph#176: what this renderer does with an -// `x-rules` node whose `kind` it does not recognise. +// The forward-compatibility half of the `x-rules` agreement: what this +// renderer does with an `x-rules` node whose `kind` it does not recognise. // // forms.md ("Renderer fallback" -> "'Cannot evaluate' means defer, not block") // settles a sentence two shipped clients once read in opposite directions. @@ -44,10 +44,10 @@ TestCase { } // Bound into the component rather than passed as an initial property. - // Both paths render the same form since morph#388 (`DynamicForm` re-reads - // the schema as JSON at the property, so the initial-properties path's - // QVariantMap conversion no longer changes what it sees); binding stays - // because it is what every shipped app does. + // Both paths render the same form (`DynamicForm` re-reads the schema as + // JSON at the property, so the initial-properties path's QVariantMap + // conversion cannot change what it sees); binding is used because it is + // what every shipped app does. property var pendingSchema: ({}) Component { diff --git a/src/qt/forms/tests/tst_LargeIdPrecision.qml b/src/qt/forms/tests/tst_LargeIdPrecision.qml index 4f2749ba..89907b7b 100644 --- a/src/qt/forms/tests/tst_LargeIdPrecision.qml +++ b/src/qt/forms/tests/tst_LargeIdPrecision.qml @@ -1,7 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // -// Ids above 2^53 must survive the renderer's JSON round trip exactly -// (morph#190, morph#191). +// Ids above 2^53 must survive the renderer's JSON round trip exactly. // // JavaScript numbers are IEEE-754 doubles, so `JSON.parse` cannot hold an // integer above 2^53, and re-serialising the rounded value emits a *different* @@ -125,11 +124,11 @@ TestCase { } } - // ── morph#191: CollectionView row ids ──────────────────────────────────── + // ── CollectionView row ids ─────────────────────────────────────────────── - // The two rows must remain addressable as distinct objects. Before the fix - // both objectNames ended in the same rounded id, so findChild could not - // name one row rather than the other. + // The two rows must remain addressable as distinct objects. Rounded through + // a double, both objectNames end in the same id and findChild cannot name + // one row rather than the other. function test_neighbouringRowIdsStayDistinct() { var view = createTemporaryObject(viewComponent, testCase) verify(view !== null) @@ -172,11 +171,12 @@ TestCase { compare(cell.text, testCase.oddId) } - // ── morph#190: choice option ids ───────────────────────────────────────── + // ── choice option ids ──────────────────────────────────────────────────── // Each option's valueJson is the literal the form submits for that choice, - // so two options must not share one. Before the fix both were the rounded - // even id, and the combo held two entries the UI could not tell apart. + // so two options must not share one. Rounded through a double both become + // the same even id, and the combo holds two entries the UI cannot tell + // apart. function test_choiceOptionValuesStayDistinct() { var form = createTemporaryObject(choiceFormComponent, testCase) verify(form !== null) diff --git a/src/qt/forms/tests/tst_i18n.qml b/src/qt/forms/tests/tst_i18n.qml index 405e2d97..618db72a 100644 --- a/src/qt/forms/tests/tst_i18n.qml +++ b/src/qt/forms/tests/tst_i18n.qml @@ -78,7 +78,7 @@ Item { }) } - // morph#583. eu_ES spells its negative sign U+2212 MINUS SIGN, not the + // eu_ES spells its negative sign U+2212 MINUS SIGN, not the // ASCII hyphen -- one of 77 locales in Qt 6.11.2 whose sign is not a bare // "-". Its separators are de-DE's, so the only thing under test here is the // sign. @@ -156,12 +156,11 @@ Item { compare(localeForm.previewLine, '{"mass":{"num":1050250,"den":1000,"dp":3}}') } - // morph#574. The de-DE locale groups with "." and this form's user - // typed the US decimal form. Stripping the group separator - // unconditionally -- which this mirror did, byte for byte in step with - // its C++ twin -- submitted 1.5 as 15: a valid payload, ten times too - // large, with nothing downstream able to tell. The field is now - // reported malformed, which is what the user can act on. + // The de-DE locale groups with "." and this form's user typed the US + // decimal form. Stripping the group separator unconditionally would + // submit 1.5 as 15: a valid payload, ten times too large, with nothing + // downstream able to tell. The field is reported malformed instead, + // which is what the user can act on. function test_foreignDecimalSeparatorIsRejectedNotAbsorbed() { localeForm.setFieldValue("mass", "1.5") verify(!localeForm.ready) @@ -182,7 +181,7 @@ Item { compare(localeForm.previewLine, '{"mass":{"num":1050250,"den":1000,"dp":3}}') } - // ── morph#583: the negative sign is locale data too ────────────── + // ── the negative sign is locale data too ──────────────────────── // // The premise, measured rather than assumed. If Qt's CLDR data ever // stops reporting U+2212 for eu_ES, this fails first and says so, @@ -221,7 +220,7 @@ Item { // one-unit comparison cannot match it at all -- ar_DZ included, whose // sign *is* the ASCII hyphen behind a U+200E. Driven through the mirror // directly: these locales' own separators are Arabic-Indic, which is a - // separate gap (morph#591), so the sign is isolated here. + // separate matter, so the sign is isolated here. function test_bidiPrefixedSignIsMatchedAsAWholeString() { compare(signForm.normalizeLocaleNumber("\u200E\u22125", { decimalSeparator: ".", groupSeparator: "", negativeSign: "\u200E\u2212" }), "-5") // fa_IR compare(signForm.normalizeLocaleNumber("\u200E-\u200E5", { decimalSeparator: ".", groupSeparator: "", negativeSign: "\u200E-\u200E" }), "-5") // az_IR @@ -253,23 +252,23 @@ Item { // Unlike a group separator, no locale is without a negative sign, so an // omitted or empty one reads as "-" rather than as absence: formatting - // -5 to "5" would be a silently wrong value, which is the morph#574 - // failure mode rather than a rejection. + // -5 to "5" would be a silently wrong value rather than a rejection -- + // the same failure mode unconditional group-stripping has. function test_anEmptySignReadsAsTheAsciiDefault() { compare(signForm.formatCanonicalNumber("-5", { decimalSeparator: ".", groupSeparator: "", negativeSign: "" }), "-5") compare(signForm.normalizeLocaleNumber("-5", { decimalSeparator: ".", groupSeparator: "", negativeSign: "" }), "-5") compare(signForm.normalizeLocaleNumber("123", { decimalSeparator: ".", groupSeparator: "", negativeSign: "" }), "123") } - // The morph#497 rule is about the *output*, so it has to hold for a - // multi-unit sign exactly as it does for "-". + // The leading-position rule is about the *output*, so it has to hold + // for a multi-unit sign exactly as it does for "-". function test_aLocaleSignIsStillRejectedOffTheLeadingPosition() { compare(signForm.normalizeLocaleNumber("1\u22122", { decimalSeparator: ".", groupSeparator: "", negativeSign: "\u2212" }), null) compare(signForm.normalizeLocaleNumber(",\u22125", { decimalSeparator: ",", groupSeparator: ".", negativeSign: "\u2212" }), null) compare(signForm.normalizeLocaleNumber("\u2212", { decimalSeparator: ".", groupSeparator: "", negativeSign: "\u2212" }), null) } - // ── morph#596: a leading positive sign is accepted, and dropped ── + // ── a leading positive sign is accepted, and dropped ──────────── // // The premise, measured rather than assumed, and on the same object the // renderer forwards from. 54 of the 711 locales Qt 6.11.2 knows spell @@ -318,8 +317,8 @@ Item { compare(localeForm.normalizeLocaleNumber("\u200E+\u200E5", { decimalSeparator: ".", groupSeparator: "" }), null) // The ASCII "+" stays accepted in a bidi-sign locale, for the same - // reason the ASCII "-" does (morph#583): the locale's own spelling - // is on no keyboard. + // reason the ASCII "-" does: the locale's own spelling is on no + // keyboard. compare(localeForm.normalizeLocaleNumber("+5", { decimalSeparator: ".", groupSeparator: "", negativeSign: "-", positiveSign: "\u061C+" }), "5") // An empty positiveSign leaves the ASCII spelling, and must not // match at every index. @@ -327,8 +326,8 @@ Item { compare(localeForm.normalizeLocaleNumber("123", { decimalSeparator: ".", groupSeparator: "", negativeSign: "-", positiveSign: "" }), "123") } - // morph#497's rule is about the *output*, so a new sign spelling must - // not open a new way to inject one. + // The leading-position rule is about the *output*, so a second sign + // spelling must not open a second way to inject one. function test_aPositiveSignObeysTheLeadingPositionRule() { compare(localeForm.normalizeLocaleNumber("1+2", { decimalSeparator: ".", groupSeparator: "" }), null) compare(localeForm.normalizeLocaleNumber("+-5", { decimalSeparator: ".", groupSeparator: "" }), null) @@ -355,7 +354,7 @@ Item { compare(localeForm.formatCanonicalNumber("-1050.25", { decimalSeparator: ",", groupSeparator: "." }), "-1.050,25") } - // ── morph#599: the separators are matched as whole strings too ─── + // ── the separators are matched as whole strings too ───────────── // // The premise, measured rather than assumed -- and it says something // different from the two sign premises above, which is the whole point @@ -368,14 +367,14 @@ Item { // negativeSign with size() > 1: 54 <- the control // positiveSign with size() > 1: 54 <- the control // - // So no locale reaches this and no user is affected. What is fixed is - // the mirror's own consistency: morph#583 and morph#596 converted the - // *signs* in this function to whole-string matching and left the - // separators as one-code-unit comparisons a few lines away, with - // nothing saying why. docs/spec/forms/forms.md, "Both edges, or - // neither": a divergence between the mirror and - // include/morph/render/locale_format.hpp is a divergence in what the - // product accepts, whether or not a locale can currently express it. + // So no locale reaches this and no user is affected. What it buys is + // the mirror's own consistency: the *signs* in this function are + // matched as whole strings, and separators compared one code unit at a + // time a few lines away would be a mixed idiom with nothing saying why. + // docs/spec/forms/forms.md, "Both edges, or neither": a divergence + // between the mirror and include/morph/render/locale_format.hpp is a + // divergence in what the product accepts, whether or not a locale can + // currently express it. // // Qt.locale() cannot enumerate, so the widest spellings the // enumeration found are pinned here one by one. If CLDR ever gives one @@ -393,7 +392,7 @@ Item { } // The control, read off the same objects: a length check on locale // data is not vacuously 1: the signs really are two and three units - // in these very locales, which is what morph#583/#596 were about. + // in these very locales. compare(Qt.locale("ar_EG").negativeSign.length, 2) compare(Qt.locale("az_IR").positiveSign.length, 3) } @@ -438,9 +437,9 @@ Item { compare(localeForm.normalizeLocaleNumber("5" + D2 + "25", { decimalSeparator: D2, groupSeparator: "" }), "5.25") } - // Whole-string matching must not loosen any of the rules the - // one-unit comparison enforced. morph#574's grouping validation and - // morph#497's leading-position rule are stated over "the separator", + // Whole-string matching must not loosen any of the rules a one-unit + // comparison enforces. The grouping validation and the + // leading-position rule are stated over "the separator", // so they have to hold when the separator is more than one unit. // // Stated plainly, because it matters for what this function is worth @@ -453,14 +452,14 @@ Item { const G2 = "\u200E." const D2 = "\u200E," - // morph#574: the last group of the integer part is short. + // The last group of the integer part is short. compare(localeForm.normalizeLocaleNumber("1" + G2 + "5", { decimalSeparator: D2, groupSeparator: G2 }), null) compare(localeForm.normalizeLocaleNumber("1" + G2 + "2" + G2 + "3" + G2 + "4", { decimalSeparator: D2, groupSeparator: G2 }), null) // Grouping belongs to the integer part only. compare(localeForm.normalizeLocaleNumber("1" + D2 + "5" + G2 + "000", { decimalSeparator: D2, groupSeparator: G2 }), null) // A second decimal separator. compare(localeForm.normalizeLocaleNumber("1" + D2 + "0" + D2 + "5", { decimalSeparator: D2, groupSeparator: G2 }), null) - // morph#497: a sign after the decimal separator is not leading. + // A sign after the decimal separator is not leading. compare(localeForm.normalizeLocaleNumber(D2 + "-5", { decimalSeparator: D2, groupSeparator: G2 }), null) // One string cannot play both roles, multi-unit or not. compare(localeForm.normalizeLocaleNumber("1" + G2 + "050", { decimalSeparator: G2, groupSeparator: G2 }), null) @@ -472,10 +471,9 @@ Item { } // The round trip, which is where "both edges, or neither" bites: - // formatCanonicalNumber has always emitted the separators as whole - // strings, so with a multi-unit separator the display edge produced - // text the entry edge then rejected -- the exact morph#583 shape, for - // a locale that does not exist yet. + // formatCanonicalNumber emits the separators as whole strings, so an + // entry edge that did not match them whole would reject text the + // display edge produced -- for a locale that does not exist yet. function test_theDisplayEdgeEmitsAMultiUnitSeparatorAndEntryTakesItBack() { const G2 = "\u200E." const D2 = "\u200E," @@ -484,9 +482,9 @@ Item { compare(localeForm.normalizeLocaleNumber(display, { decimalSeparator: D2, groupSeparator: G2, negativeSign: "\u2212" }), "-1050.25") } - // --- morph#591: the digits are locale data too -------------------- + // --- the digits are locale data too ------------------------------- // - // The same corpus the C++ edge pins under [morph591] in + // The same corpus the C++ edge pins in // tests/test_render_locale_format.cpp. docs/spec/forms/forms.md, // "Both edges, or neither": a row that disagrees between the two lists // is a divergence in what the product accepts. @@ -560,7 +558,7 @@ Item { } } - // The morph#596 precedent, applied to digits: the locale's own digits + // The positive-sign precedent, applied to digits: the locale's own digits // are on the user's keyboard only if their keyboard has them, so entry // accepts a spelling display never produces. function test_asciiDigitsStayAcceptedInANativeDigitLocale() { @@ -600,8 +598,8 @@ Item { } // An omitted or empty zeroDigit reads as ASCII "0", the reading an - // omitted negativeSign gets, and every caller that names no zeroDigit is - // byte-identical to what it produced before morph#591. + // omitted negativeSign gets, so a caller that names no zeroDigit gets + // the plain ASCII behaviour. function test_anAbsentZeroDigitReadsAsAscii() { compare(localeForm.normalizeLocaleNumber("55", { zeroDigit: "" }), "55") compare(localeForm.formatCanonicalNumber("55", { zeroDigit: "" }), "55") diff --git a/src/qt/forms/tests/tst_main.cpp b/src/qt/forms/tests/tst_main.cpp index 7180f9b2..dbeb6289 100644 --- a/src/qt/forms/tests/tst_main.cpp +++ b/src/qt/forms/tests/tst_main.cpp @@ -21,9 +21,9 @@ namespace { /// /// Each corpus is one file with two readers — a C++ suite and a QML one: /// -/// - `tests/data/rule_corpus.json` (morph#176) — read by +/// - `tests/data/rule_corpus.json` — read by /// `tests/test_forms_rule_corpus.cpp` and `tst_DynamicFormRuleCorpus.qml`; -/// - `tests/data/instance_bounds.json` (morph#164) — read by +/// - `tests/data/instance_bounds.json` — read by /// `tests/test_forms_instance_constraints.cpp` and /// `tst_DynamicFormInstanceBounds.qml`. /// diff --git a/src/qt/qt_websocket_backend.cpp b/src/qt/qt_websocket_backend.cpp index 92ad7076..3e42f4aa 100644 --- a/src/qt/qt_websocket_backend.cpp +++ b/src/qt/qt_websocket_backend.cpp @@ -47,7 +47,7 @@ QtWebSocketBackend::QtWebSocketBackend(QUrl serverUrl, ::morph::model::detail::A _connectHandler(); } // Send every bind request that arrived before this connect (the first - // connect included) -- see issue #54. Runs before the reconnect handler + // connect included). Runs before the reconnect handler // below so a caller that gates UI on the bind's continuation sees it // fire promptly on first connect too. flushQueuedRegistrations(); @@ -187,7 +187,7 @@ ::morph::async::Completion<::morph::exec::detail::ModelId> QtWebSocketBackend::b if (!_connected) { // Queue rather than fail: this is exactly the ordering a // single-threaded WASM client must use, since it can never block - // waiting for the connection to settle (see issue #54). The queued + // waiting for the connection to settle. The queued // request is sent -- with a call-id assigned then, not now -- the // moment `connected` fires next (first connect included), from // flushQueuedRegistrations(). No call-id is assigned yet; if the @@ -229,10 +229,10 @@ void QtWebSocketBackend::sendControl(::morph::wire::Envelope env, uint64_t const callId = ++_nextCallId; env.callId = callId; // Stamped exactly as every synchronous control verb stamps it: RemoteServer - // authenticates and authorizes from env.session, so omitting it reached the - // server as an unauthenticated principal on the non-blocking (WASM) path - // only. morph#495 -- and now unmissable, because this is the only place a - // control envelope is sent from. + // authenticates and authorizes from env.session, so omitting it would reach + // the server as an unauthenticated principal on the non-blocking (WASM) + // path. Unmissable here, because this is the only place a control envelope + // is sent from. env.session = _session; QString encoded; try { @@ -374,7 +374,7 @@ void QtWebSocketBackend::deregisterModel(::morph::exec::detail::ModelId mid) { if (!_connected) { return; } - // A real, non-zero callId (issue #65): callId == 0 is the wire's + // A real, non-zero callId: callId == 0 is the wire's // "parked sendSync waiter" sentinel, and this request's reply -- though // nobody waits on it -- would otherwise be indistinguishable from one a // synchronous register/attach/assign/instances call is genuinely parked @@ -443,14 +443,13 @@ void QtWebSocketBackend::cancelPending(const std::exception_ptr& exc) { } for (auto& [ignoredCallId, promise] : drainedRegistrations) { // The exception itself, not a message rebuilt from it: a control call - // rejected by a dropped socket now delivers the very - // `backend::DisconnectedError` an execute() call delivers, instead of - // the `runtime_error` the `*Async` verbs' string channel flattened it - // into. + // rejected by a dropped socket delivers the very + // `backend::DisconnectedError` an execute() call delivers, so a caller + // can catch one type for both. promise.reject(exc); } - // A private bind queued while the socket had never yet connected (issue - // #54) never got a call-id, so it cannot be found in _pendingRegistrations + // A private bind queued while the socket had never yet connected never got + // a call-id, so it cannot be found in _pendingRegistrations // above -- drain it here instead, on the same cancelPending path that // already handles a connection that goes away (or never comes up) before a // queued reply, so its continuation still fires exactly once rather than @@ -474,7 +473,7 @@ void QtWebSocketBackend::scheduleReconnect() { // Cast up to double first so the multiplication is openly floating-point. // Written as `count() * backoffMultiplier` the integral `rep` is narrowed to // double *inside* the expression, which the narrowing-conversions checks - // flag separately from the explicit cast back (morph#514). Same arithmetic, + // flag separately from the explicit cast back. Same arithmetic, // same result -- only the one deliberate narrowing is left, on the outside. auto next = std::chrono::milliseconds{static_cast( static_cast(_currentReconnectDelay.count()) * _cfg.backoffMultiplier)}; @@ -576,7 +575,7 @@ bool QtWebSocketBackend::tryRouteControlReply(const ::morph::wire::Envelope& env } bool QtWebSocketBackend::tryRouteDeregisterReply(const ::morph::wire::Envelope& env) { - // A fire-and-forget deregister's reply (see issue #65): assigned a + // A fire-and-forget deregister's reply: assigned a // real callId purely so it lands here instead of falling through // to the callId==0 branch in onTextMessage and being handed to whichever // sendSync waiter happens to be parked. Nobody observes the diff --git a/src/qt/qt_websocket_server.cpp b/src/qt/qt_websocket_server.cpp index 61ce9e55..5668750f 100644 --- a/src/qt/qt_websocket_server.cpp +++ b/src/qt/qt_websocket_server.cpp @@ -179,7 +179,7 @@ void QtWebSocketServer::onNewConnection() { // Qt parent-child ownership: `this` owns the timer and deletes it, and every path that // drops the reference calls deleteLater() first (see the handshakeTimer handling below). // cppcoreguidelines-owning-memory has no model of that convention -- it flags any raw - // `new` bound to a non-gsl::owner pointer (morph#514). Suppressed here rather than for the + // `new` bound to a non-gsl::owner pointer. Suppressed here rather than for the // whole directory: this is the only such site in src/, and a directory-wide disable would // turn the check off for a future `new` that really is unowned. // NOLINTNEXTLINE(cppcoreguidelines-owning-memory) — QObject parent owns this @@ -260,7 +260,7 @@ void QtWebSocketServer::onTextMessage(const QString& message) { // an ordinary error the caller's `.onError(...)` already handles -- // the same reply-without-full-decode pattern the maxMessageBytes // branch above uses. The connection stays open: rate limiting throttles - // a client, it does not evict one (morph#225). + // a client, it does not evict one. socket->sendTextMessage( QString::fromStdString(::morph::wire::encode(::morph::wire::makeErr("rate limited", peekedCallId())))); return;