Record the seventh sweep, over the rigs and the sixth sweep's own fix
Six findings in iris f8aa0c5, and the seven things that tripped a rule and were left. Three of those are named as work waiting for the pre-review-gate pass, since they pre-date #19 and are outside its diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
29dd84e04e
commit
5b34d406c7
2 files changed
+133
-10
No files matched your search
@@ -6,6 +6,117 @@ nothing here is rediscovered. Each entry says who found it and when.
|
||||
it (settled design, the measurement method) belongs in `docs/LAYOUT.md`, and
|
||||
the current plan is in `docs/HANDOFF.md`.
|
||||
|
||||
## Seventh sweep: the rigs, `Fixed`, and the sixth sweep's fix (2026-09-20)
|
||||
|
||||
Over what no earlier round named -- `src/random.rs` and `tests/scenario/`,
|
||||
`core/src/fixed.rs`, `scripts/run-headless.sh` -- and once more over
|
||||
`b7b8d09`, which was itself unreviewed because it was the sixth sweep's own
|
||||
fix. Six findings, all in `f8aa0c5`. The cold dump over 400 depth-5 trees is
|
||||
byte-identical to `b7b8d09` across all 34,492 boxes, and all three seed scans
|
||||
pass (400 at depth 5 in 62.75s, 1,000 at depth 6 in 162.37s, 2,000 at depth 4
|
||||
in 305.25s).
|
||||
|
||||
**A scroll still asking a question it has already answered.** The sixth
|
||||
sweep found `anchor != Px::ZERO` in `Scroll`'s content test and removed it;
|
||||
the operand beside it is the same defect and survived. `self.content_len` is
|
||||
`answer_px.max(container_len)`, so content that fits has nothing to scroll
|
||||
through, and `update_amt` on the line above clamps `amt` to
|
||||
`content_len - container_len`, which is then zero. So in
|
||||
|
||||
let content = match self.amt != Px::ZERO || self.content_len != self.container_len {
|
||||
|
||||
the first disjunct can never decide the match: `amt != ZERO` implies
|
||||
`content_len != container_len`, which the second already tests. It is now
|
||||
`match self.content_len > self.container_len`, the exact complement of the
|
||||
`content_len <= container_len` that guards the contract fifteen lines above.
|
||||
Verified by asserting the implication in place and running the whole suite,
|
||||
the scrolling tests included; it held. This is the lesson of "the fixes a
|
||||
review produces are themselves unreviewed code" arriving on schedule -- one
|
||||
round's fix left the same mistake in the expression it was editing.
|
||||
|
||||
**Things nothing reads, in the new arithmetic.** `Fixed::to_scale` converts a
|
||||
value between two fixed-point grids, and `shift_round` exists only to serve
|
||||
it. Both arrived on this branch; the only caller either has ever had is
|
||||
`a_coarser_grid_rounds_and_a_finer_one_does_not`, the test written for them.
|
||||
Every other `Fixed` method has real callers (checked one by one), and nothing
|
||||
needs a grid conversion: `ratio`, `mul` and `div` already cross grids where
|
||||
layout has to. All three deleted. `to_scale` was also the one operation in
|
||||
the file that could overflow silently without documenting it -- going to a
|
||||
finer grid is `self.0 << (TO - SHIFT)` and the test only ever went coarse,
|
||||
fine, coarse with a value small enough to survive the trip.
|
||||
|
||||
**`Len` arithmetic written a component at a time.** `Len::align` built a
|
||||
`Len` whose `px` is always `Px::ZERO`, then added to and subtracted from both
|
||||
of its components by hand:
|
||||
|
||||
start: Len::from_parts(at.rel.sub(self.rel.mul(rel)), at.px.sub(self.px.mul(rel))),
|
||||
end: Len::from_parts(at.rel.add(self.rel.mul(rest)), at.px.add(self.px.mul(rest))),
|
||||
|
||||
`Len::scale` is "both parts by the same fraction" and `Len` has `Add` and
|
||||
`Sub`, so the whole rule is `at - self.scale(rel)` and
|
||||
`at + self.scale(Rel::ONE.sub(rel))` -- the point the alignment names, less
|
||||
the part of the length before it. Identical arithmetic, which the dump
|
||||
confirms. It is the only place in the codebase that expanded a `Len`
|
||||
operation like this; `LayoutLen::apply_leftover` touches `rel` alone, which
|
||||
is genuinely one component.
|
||||
|
||||
**A question asked through a value, one line from its sibling.** `445287c`
|
||||
moved every method that answers a question about a value to `&self`.
|
||||
`LayoutLen::without_leftover` was missed, and its own doc comment calls
|
||||
`apply_leftover` -- which takes `&self` -- "the opposite reading of the same
|
||||
value". Now `&self` too. Every other by-value method in the workspace that
|
||||
does not return `Self` was checked: the rest are conversions on numbers
|
||||
(`Fixed::raw`, `to_f32`) or builders.
|
||||
|
||||
**A rig that scales a gesture against a mode the output no longer has.**
|
||||
`run-headless.sh --mode` sets `out_w`/`out_h` beside the `swaymsg output ...
|
||||
mode`, because those two numbers are the extent `replay-touch` passes to
|
||||
`zwlr_virtual_pointer_v1::motion_absolute` -- the recording's coordinates are
|
||||
a fraction of them. `--resize`, added in `b7b8d09`'s round, changed the mode
|
||||
and left the extent alone, so `--resize 800x600@60Hz --replay flick.touch`
|
||||
replayed every sample at `x * 800 / 1920` and finished looking like a run
|
||||
that worked. Both are one `set_mode` function now, so a third mode change
|
||||
cannot get it wrong. Found by reading rather than by running: this VM has no
|
||||
recorded gesture that also resizes, which is exactly why it went unnoticed.
|
||||
|
||||
**A comment the plan/build split stranded.** `src/random.rs`'s "a row takes
|
||||
the height it is given rather than its tallest child" sat above
|
||||
`let gap = self.rng.below(3) as i32 * 4`, telling a reader that the line
|
||||
consumes no randomness. It describes the `set_size_rules` that `98d4e98`
|
||||
moved into `Build::kind`, and the line it was left above is one of the two
|
||||
draws in the function. Moved to the rule it is about, and the
|
||||
seed-stability half dropped: building a plan consumes no randomness at all
|
||||
now, so there is nothing left for that clause to say.
|
||||
|
||||
### Tripped a rule and left as it stands
|
||||
|
||||
- `Align::tuple`, `UiVec2::partial_align` and `Vec2::partial_align` have no
|
||||
callers anywhere. All three pre-date this PR and are outside its diff, so
|
||||
they belong to the pre-review-gate sweep rather than to this branch.
|
||||
- `Vec2::align`/`partial_align` are `UiVec2`'s two functions again with
|
||||
`UiVec2::from(*self)` in front. Also pre-existing, and merging them means
|
||||
deciding whether `Vec2` should have them at all.
|
||||
- `impl_op!` has four grammars for one macro, and `core/src/util/vec2.rs`
|
||||
spells two of them one line apart -- `impl_op!(impl Add for Vec2: add x y)`
|
||||
beside `impl_op!(Vec2 Sub sub; x y)`. The `impl ... for ...:` arm has that
|
||||
one caller. Pre-dates this PR; the two arms this PR added (`same ...`) are
|
||||
a real distinction, since a type mixing a fraction and an offset has no
|
||||
meaning for a bare `f32`.
|
||||
- `AxisAlign`, `RegionAlign` and `Align` lost `Eq` when `AxisAlign` became a
|
||||
`Rel` wrapper, which `Rel` derives. Nothing needs it, so it was left rather
|
||||
than adding a derive with no reader.
|
||||
- `Holds::through` special-cases a fully unbounded range before inverting a
|
||||
fraction, which the general path would also answer correctly through
|
||||
`narrow`. Left: it is the one place a `Px::MIN`-to-`Px::MAX` interval is
|
||||
shifted and divided, and the early return says that no fraction can narrow
|
||||
"every length".
|
||||
- `Scroll`'s `content_len <= container_len` can only be equality, given the
|
||||
`max` that built it. Left: `<=` reads as "the content fits", which is what
|
||||
the branch means, and the mirror `>` now reads as "it overflows".
|
||||
- `scenario::Case::name`, `window` and `Shuffle::of` take `self` on `Copy`
|
||||
enums. They are test-rig code the `&self` pass did not cover, and an enum
|
||||
with no fields is the one place taking a value costs a caller nothing.
|
||||
|
||||
## Sixth sweep: the shader boundary and the position widgets (2026-09-20)
|
||||
|
||||
Over what the five earlier rounds did not name -- the WGSL prelude and how
|
||||
|
||||
Reference in new issue
Block a user