Two review findings this lane could not fix, written down
This commit is contained in:
parent
2d29528cad
commit
dd239d6cd5
58
FIX.org
58
FIX.org
@ -858,3 +858,61 @@ skipping its stores on a re-run is safe against all of that because the
|
||||
pushes take the global's slot address, never its value, and they sit before
|
||||
the startup call on both backends — nothing in the bracket depends on an
|
||||
initialiser having run.
|
||||
|
||||
* Review-batch findings, 2026-09-20
|
||||
Two things found in review that this lane could not fix in the files it owned.
|
||||
Written down here so they are not lost with the branch.
|
||||
|
||||
** The daemon leaves its temp directory behind, forever
|
||||
Every session makes =/tmp/flan-dev-<pid>/= — lib/dev.ml:3516 for the
|
||||
two-process daemon and lib/dev.ml:4415 for the merged one — and nothing ever
|
||||
removes it. It holds the built =program=, the host's =host.ll= or =host.s=,
|
||||
the reload modules and =agent.sock=: about 7MB a session. The review counted
|
||||
873 of them, 5GB, on the morning of 2026-09-19; this lane counted 68 and 457MB
|
||||
on 2026-09-20. Whatever removed the difference, nothing in the tree did, and
|
||||
the count climbs again with every =M-x flan=.
|
||||
|
||||
The fix is small and the merged daemon already has the one place for it. Its
|
||||
session ends at lib/dev.ml:4395: [accept_loop] returns, the listening socket
|
||||
closes, the editor socket is unlinked, and [Unix._exit 0] follows. A recursive
|
||||
remove of [dir] belongs between the unlink and the flush — that one site
|
||||
covers all three ways a session ends cleanly, because all three come back
|
||||
through [accept_loop]:
|
||||
- =close= from the editor (lib/dev.ml:3359, which returns [true] and ends the
|
||||
loop);
|
||||
- no editor connected for the grace period (lib/dev.ml:3472);
|
||||
- the program finished and the parked process is let go.
|
||||
The two-process daemon needs the same thing at its own session end.
|
||||
|
||||
Two deliberate non-goals, and they are the reason this is worth spelling out
|
||||
rather than just doing:
|
||||
- *Not on a crash.* The sibling branch at lib/dev.ml:4393 is [accept_loop]
|
||||
raising, and the directory is the post-mortem — the binary and the exact IR
|
||||
it was built from. Only the clean return cleans up.
|
||||
- *Not other sessions' directories.* A sweep of =/tmp/flan-dev-*= would delete
|
||||
the working directory of a daemon that is still running, and a stale pid is
|
||||
not proof of anything. Each session removes its own and no more.
|
||||
|
||||
Not done here because lib/dev.ml belongs to another lane that has not merged.
|
||||
|
||||
** and's last operand gets a misdirected caret in a want-free position
|
||||
[shortcircuit] in lib/parse.ml documents this at the site; the summary is that
|
||||
=(println (and true true (vec-new i32)))= reports "expected (Vec i32), found
|
||||
bool" with the caret on the second =true=. The last operand of an =and= is the
|
||||
then arm, check_if types the then arm first, and the mismatch is therefore
|
||||
reported against the else arm, which carries the *previous* operand's loc.
|
||||
Every other operand position is right, because an operand anywhere but last is
|
||||
a condition and check_truthy blames it at its own loc; =or= is right
|
||||
everywhere, because there the chain and not the sentinel sits in the else arm.
|
||||
Compiled on the tree at every operand position of both forms, want-free and
|
||||
want-ful; want-ful is right everywhere too, because the want reaches each arm
|
||||
instead of the arms being unified against each other.
|
||||
|
||||
Not fixed. Three candidate fixes were considered and rejected: giving the else
|
||||
arm the last operand's loc makes the sentence read backwards ("expected (Vec
|
||||
i32)" under a caret on the thing that is the (Vec i32)); answering a bool
|
||||
sentinel again reverts the fix that made =(or nil "x")= answer ="x"=; and
|
||||
inverting the condition to move the last operand into the else arm costs a
|
||||
[not] per operand and worse locs than it buys. What would fix it is check_if
|
||||
preferring the arm that is not a compiler temp when it decides which one to
|
||||
blame — a change in check.ml, which this lane did not own.
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user