diff --git a/AGENTS.md b/AGENTS.md index c03c692fb..eee1d8657 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 diff --git a/include/morph/core/async.hpp b/include/morph/core/async.hpp index dc4f63579..6c01488fa 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 | /// |---|---|---| @@ -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 c321ee9ef..95b39fc57 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 6535cb2e6..d79c3738c 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 62a9380ca..6042fd537 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 445a3fcc5..7adc66726 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 952ef3b56..7fb586570 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 121748791..9aa286754 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 16bb39368..ce8dacf44 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 b324efe11..be600b11f 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 057ef2be7..cb18fc9a6 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 e89c75454..295028188 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 4c686a70c..249f23fbe 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 a658c7347..c069352f2 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 ac6172a20..c403234b6 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. diff --git a/include/morph/detail/quantity_equation.hpp b/include/morph/detail/quantity_equation.hpp index 11b57d7ae..3cad511bc 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 faa367b58..685f87cc5 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 2c192d673..a0a5eb770 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 86317b61c..97bc6a23b 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 c54d69794..e50b06d36 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 a96b3b68f..b42a8aee7 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 d95f26c87..06fbbf59c 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/journal/action_log.hpp b/include/morph/journal/action_log.hpp index c9936e2fd..774945525 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 30e36148a..94b77323a 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 1a46e7f6d..568fc731e 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 cdd0789cf..8be9d5d93 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 c6f5a8db2..7151c7bb9 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/net/detail/tcp_socket.hpp b/include/morph/net/detail/tcp_socket.hpp index ea184d2dd..d9365d9cc 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 b17aceee8..ac7fd8e9b 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 c036c6d88..2464ed67d 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 c1e0e7dd9..40e0f5956 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 aa9960185..38960d970 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 48ac65700..d74131fd7 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/qt/forms/forms_controller_core.hpp b/include/morph/qt/forms/forms_controller_core.hpp index 8ff468e18..658f834b8 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 d830fad59..502fd43cc 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 95556924f..702f3217a 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 6edb2c3f9..44c5bac15 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 e2c67d491..a43cf52b2 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 016f6cc1a..93cfe7ef8 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 c6c1cda55..80b705548 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; diff --git a/include/morph/util/quantity.hpp b/include/morph/util/quantity.hpp index de0da57ff..ea4497c92 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 diff --git a/src/qt/forms/CMakeLists.txt b/src/qt/forms/CMakeLists.txt index f301eabe5..c5f9963a9 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 b947d49c7..d11abbfb0 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 f5c4b4f2d..d6e854c93 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 b4ab3840b..384257ae7 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 ff8060b11..41aeda790 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 876b515fb..1e90202af 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 1b26c0903..d149bca20 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 c0ecf5309..5df2e452f 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 87e29c918..f94fc8a39 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 419c16cce..dbe108000 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 d655d8d93..b0b860411 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 fc8fcca60..134de0ed1 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 d824798a5..ccb94aa94 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 106739100..457c08730 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 03370e969..ded0fd994 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 2aff18bb5..e16758b79 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 44262c296..38d73c96e 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 56c6a77af..aaa3f1cf7 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 4f2749bac..89907b7b0 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 405e2d97b..618db72a8 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 7180f9b26..dbeb6289b 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 92ad70765..3e42f4aa9 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 61ce9e552..5668750f2 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;