1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
|
# Source code review checklist
Software Level: **DAL-C** (DO-178C)
Reviewer: author — **review without independence**, which DO-178C permits at
Software Level C.
Scope: `src/buffer/**`, `src/components/buffer.rs`, `src/config.rs`,
`src/app.rs`, `src/cli.rs`, `src/action.rs`
Derived from DO-178C Table A-5 (verification of the source code), reduced to
the objectives that apply at DAL-C.
---
## Compliance and traceability
| # | Check | Result |
|---|---|---|
| 1 | Source complies with the low-level requirements | **Pass** |
| 2 | Source is traceable to the LLRs | **Pass** — 98/98 tagged `// MJB-LLR-nnn`, verified by `grep`, not inspection |
| 3 | Source complies with the software architecture | **Pass** — buffer core has no `ratatui` dependency and is testable headless |
| 4 | Source is verifiable | **Pass** — 96.21% statement coverage of `src/buffer/**` |
| 5 | Source conforms to a coding standard | **Pass** — `cargo clippy --all-targets -- -D warnings` is clean |
| 6 | Source is accurate and consistent | **Pass** — see the defects found, below |
Reproduce objectives 2, 4 and 5:
```bash
cargo clippy --all-targets -- -D warnings # clean
cargo test # 294 passing
cargo llvm-cov --summary-only test
```
---
## No dead code
| # | Check | Result |
|---|---|---|
| 7 | Removed widgets leave no unreachable code | **Pass** |
| 8 | No `allow(dead_code)` masks anything | **Pass** |
| 9 | Removed dependencies are gone from the graph | **Pass** |
Evidence:
```bash
# The widget modules are deleted, not merely unregistered.
ls src/components/ # buffer.rs only
grep -rni "FpsCounter\|mod fps\|mod home" src/ # only a comment in app.rs
# No dead-code suppression anywhere in the crate.
grep -rn "allow(dead_code)" src/ # nothing
# The chord-timeout state is gone, not just unused.
grep -rn "last_tick_key_events" src/ # nothing
# JSON is out of the dependency graph, not merely out of the call path.
cargo tree | grep -iE "json|yaml|ini\b" # nothing
```
Three items of inherited dead code were removed during review, beyond the two
widgets the task named:
- `Action::Help` — declared by the application template, never produced or
handled.
- `Command::leaves_insert_mode` — written by me during implementation, then
never called. Caught because `buffer/command.rs` showed 0% coverage.
- `#![allow(dead_code)]` in `src/tui.rs` — verified to mask nothing before
removal, by deleting it and re-running clippy.
---
## Robustness and error handling
| # | Check | Result |
|---|---|---|
| 10 | No panic on malformed input | **Pass** — invalid UTF-8 is a reported error (MJB-LLR-118); a declared non-UTF-8 encoding substitutes U+FFFD (MJB-LLR-113) |
| 11 | No panic on malformed configuration | **Pass** — a bad key sequence is a recoverable error naming itself (MJB-LLR-183) |
| 12 | No panic on a failed file operation | **Pass** — every `save` path returns `Result` |
| 13 | Boundary conditions handled | **Pass** — empty buffer, no trailing newline, zero-height viewport, scrolloff exceeding the viewport, undo past history start |
| 14 | Integer overflow/underflow guarded | **Pass** — `saturating_*` throughout `view.rs` and `movement.rs` |
`unwrap`/`expect`/`panic!` outside test code is confined to two files, neither
in the buffer core:
| File | Count | Assessment |
|---|---|---|
| `src/errors.rs` | 1 | Inside the panic hook itself |
| `src/tui.rs` | 2 | Template code in `Drop`; pre-existing |
`src/buffer/**` contains **zero** panicking calls outside `#[cfg(test)]`.
The one genuinely new failure mode created by this change — a byte offset
landing inside a character, which makes ropey panic — is guarded in two places:
`Range::clamped` (MJB-LLR-011) snaps every externally supplied offset onto a
char boundary, and `ChangeSet::apply` (MJB-LLR-044) validates every operation
boundary *before* mutating, so a rejected change leaves the rope untouched
rather than half-applied.
---
## Defects found and fixed during review
Eight defects were found by review and test activity rather than by writing
the code. Each records how it was found, because that tells the next person
which verification activities are actually working (docs/process.md §6, Rule 4).
1. **Every shifted key binding was shadowed by its lowercase twin.**
`parse_key_event` lowercased the entire key string, so `"<A>"` and `"<a>"`
both parsed to `KeyCode::Char('a')`. `A`, `I`, `O`, `U`, `W`, `B` and `E`
were all unreachable, and whichever binding won the hash map ran instead —
so `a` silently invoked `InsertAtLineEnd`. Found by seven integration tests
failing at once. Fixed by preserving the final token's case and inferring
`SHIFT` from an uppercase letter, matching what crossterm reports.
2. **UTF-16 files would have been corrupted on save.**
`encoding_rs::Encoding::encode` refuses to encode to UTF-16 and silently
substitutes UTF-8, so a UTF-16 document would have been written as UTF-8
bytes beneath a UTF-16 BOM. Invisible to inspection; found only by a
byte-comparing round-trip test. Fixed by encoding UTF-16 directly from
`str::encode_utf16`. Recorded as **MJB-DR-007**.
3. **`o` on a file with no trailing newline put the cursor in the wrong place.**
`open_below` anchored to the next line's start, which does not exist on a
final unterminated line. Found by clippy's `if_same_then_else` flagging the
suspicious `if above { at } else { at }`, then confirmed by a robustness
test. Fixed by anchoring to the line's content end.
4. **`a` appended in the wrong position.** Helix's normal-mode cursor is a
one-grapheme range, so `a` lands past that grapheme; mojibake's empty range
has `to() == from()`. Fixed by stepping forward one grapheme when the range
is empty.
A later quality pass over the same code found four more. Note that three were
in paths no test covered and one was in inherited template code that only
became reachable when the buffer started consuming `[styles]`.
5. **Counted word motion was broken.** Each iteration of the count loop
re-derived its start from the partial range's *cursor*, which for a forward
range is one grapheme behind the head. `2w` on `"aaa bbb ccc"` therefore
returned `Range(3, 4)` — anchor in the wrong place, head stalled inside the
first gap — instead of `Range(0, 8)`. Found by restructuring `word_move`
into three named functions and checking each against a worked example; no
test covered a count greater than one on a word motion. Fixed by holding the
anchor fixed and advancing only the head. Regression tests
`mjb_llr_065_counted_next_word_start_advances_once_per_count` and siblings.
6. **Three panics reachable from user configuration.** `parse_color` indexed
`rgb` operands unchecked and added into `u8` unguarded, so `"rgb"`,
`"rgb1"`, `"gray99"` and `"rgb999"` in a `[styles]` block aborted the
editor at startup — a direct violation of MJB-HLR-018. Found by running
clippy with `-W clippy::pedantic` and following up the
`cast_possible_truncation` hits by hand. Fixed by bounds-checked parsing and
clamping to the actual xterm palette ranges. Regression test
`mjb_llr_183_malformed_colours_never_panic`.
7. **Bright colours were indistinguishable from their base colours.** The
template wrote `c.wrapping_shl(8)`, which on a `u8` masks the shift to
`8 % 8 == 0` and returns the value unchanged. Found while fixing defect 6.
Fixed to the ANSI convention of base + 8, saturating.
8. **The modified flag stayed set after undoing back to the saved state.** A
latched `bool` cannot distinguish "edited" from "edited and reverted", so a
buffer byte-identical to its file still refused `:q`. Found by review of the
flag's lifecycle rather than by a test. Replaced with a comparison against
the history revision the file was written at.
The first attempt at that fix was itself wrong and worth recording: using
the history *cursor* as the revision means saving at depth 3, undoing, then
making a **different** edit returns to depth 3 while the content differs —
reporting clean when it is not. Caught by reasoning through the case before
committing, and fixed by giving each history entry a unique, never-reused
id. Regression test `mjb_llr_116_revision_is_not_stack_depth`.
---
## Architecture
| # | Check | Result |
|---|---|---|
| 15 | The pre-release rope dependency is confined | **Pass** — `ropey::Rope` is owned only by `buffer/document.rs`; other modules take `RopeSlice` parameters (MJB-DR-006) |
| 16 | The buffer core is independent of the UI | **Pass** — no `ratatui` import under `src/buffer/` |
| 17 | Pagination is structural, not an optimisation | **Pass** — `View::visible_lines` is the only path to line content, and is bounded by viewport height |
Objective 17 is the one most worth stating plainly: MJB-HLR-012 is satisfied
because there is no code path that walks the document, not because a fast path
was added. `mjb_llr_200_renders_only_the_visible_window` checks the observable
consequence, and `mjb_llr_092_scrolling_a_large_file_stays_responsive` checks
that 500 half-page scrolls over a 200,000-line file stay well inside a time
budget a full scan could not meet.
---
## Summary
Code review **complete**. Eight defects found and fixed; three items of dead
code removed; no `allow(dead_code)` remains; clippy clean at `-D warnings`;
294 tests passing; 96.21% statement coverage of the buffer core.
Outstanding items are the six open derived requirements in
`docs/requirements/derived.md`, which need a safety-assessment judgement rather
than a code change. MJB-DR-001 is resolved: UTF-8 is now validated strictly and
malformed input is refused, so the lossy-save hazard it flagged is gone.
|
