chore(core,vm): drop the two dead extension includes and 29 stale externs (#744, items 1-2) - #811
Conversation
…alSystems#744) Takes the two cheapest items of the InauguralSystems#744 modularity review, verified independently of the issue's claims: 1. `eigenscript.c` included `ext_db_internal.h` and `model_internal.h` and used nothing from either. Confirmed by set-difference: of the 511 identifiers those two headers add to the TU's preprocessed token set over `eigenscript.h` + `vm.h`, the 18 that also appear in `eigenscript.c` are all parameter/member names inside declarations (`buf`, `query`, `from`, ...) — zero declared entities are referenced. This was the only core->ext include edge in the tree, and it forced libpq's headers onto the core TU: before this change, `eigenscript.c` compiled in the `full` configuration without `-I/usr/include/postgresql` fails with `fatal error: libpq-fe.h: No such file or directory`; after, it compiles clean. 2. `vm.c` carried 32 `extern` re-declarations of symbols already declared in `eigenscript.h`, free to drift from it. 29 are removed. Three are NOT redundant — `builtin_free_val`, `env_hash_find_dict` and `env_get_assign_count` have no declaration in any header and are called by `vm.c`, so they stay with a comment saying why. Of the 29, `val_incref`/`val_decref`/`num_guard` were inert (`static inline` in `eigenscript.h`) and `observer_ensure_fresh` still carried a comment pointing at `eval.c`, the TU the bytecode VM replaced. Behavior-neutral by construction, and measured: `build/{release,full, http,gfx}/{eigenscript,vm}.o` are byte-for-byte identical to the same objects built from the parent commit, and the release suite log diffs to zero lines against its baseline. Explicitly out of scope: items 3-5 of InauguralSystems#744 (the per-layer header split, the `eigenscript.h` umbrella, moving `read_file_util` out of the builtins leaf, and the `vm.c` / `builtins.c` splits). Nothing in this commit touches a header, a test, or the build config. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
InauguralPhysicist
left a comment
There was a problem hiding this comment.
Approving. Re-verified your byte-identity claim against current main (8de9715 — two merges past your base, #810 having since rewritten regions of both files): merged this branch onto it locally, rebuilt release, and cmp says vm.o, eigenscript.o, and the linked binary are all byte-identical to main's build. Declaration-only survives the moved base, so the object evidence closes the question and the suites are redundant by construction.
The 29-vs-32 correction is appreciated and checks out — the three survivors (builtin_free_val, env_hash_find_dict, env_get_assign_count) genuinely have no header home, and parking them behind a three-line block with the reason is the right call until #744's header work gives them one. The whole-header identifier-set diff on the include removal is a nicer proof than the issue's symbol table asked for. Noted for #744's remaining items: the phantom g_null_singleton_external_decl extern you couldn't find a definition for is exactly the kind of fossil the header split should sweep.
Merging.
What does this PR do?
Takes the two cheapest items of #744 — the dead extension includes in
eigenscript.cand the staleexternblock invm.c— and leaves the header-split work alone. Declaration-only; the object files are byte-identical to the base.Related Issues and Pull Requests
Relates to #744
Deliberately partial: this is items 1 and 2 of the issue's cheapest-fix-first ordering. The per-layer header split and the
eigenscript.humbrella are untouched, so #744 stays open.Changes
src/eigenscript.c: removed the guardedext_db_internal.handmodel_internal.hincludes (:12-17). This drops the only core→ext include edge, and with it the core TU's libpq header dependency.src/vm.c: removed 29 of the 32externre-declarations (the block sits at:300-335on currentmain, not the issue's:242-278— Build: every variant writes src/eigenscript, which is why the test runner needs a binary fingerprint guard — a per-variant objdir retires both #740/build: per-variant objdirs — variants coexist, rebuilds go incremental (#740) #808 moved it), replacing them with a three-line block plus a comment saying why the survivors are there.Correction to the issue's premise: it is 29, not 32. Three of them are load-bearing and stay —
builtin_free_val,env_hash_find_dictandenv_get_assign_countare called fromvm.c(:2216,:3522,:5492,:510,:4784) and no header declares them. Giving them a header home is header work, so it belongs with the part of #744 this PR does not touch. Anyone reading the issue text alone will expect a 32-line deletion, hence flagging it here.Of the 29 removed, 26 have a matching declaration in
eigenscript.h(make_num:914,env_new:1050,observer_ensure_fresh:1157,val_type_name:1175, …) and 3 —val_incref,val_decref,num_guard— arestatic inlinedefinitions there (:976,:984,:955), so theexternwas inert and arguably ill-formed (one identifier with both internal and external linkage in a single TU). No type mismatch anywhere in the block: both declarations were visible in the same TU at the merge-base and gcc and clang accept it, so compatibility already held rather than being eyeballed. One of the removed comments claimedobserver_ensure_freshcame fromeval.c— a TU the bytecode VM replaced.Testing
Merge-base
cd9f823.Behavior-neutrality, proven exactly rather than argued: I snapshotted
build/{release,full,http,gfx}/{eigenscript,vm}.ofrom the unmodified merge-base, applied the change, rebuilt, andcmp'd — all 8 object pairs byte-identical, with both TUs confirmed to have genuinely recompiled. That is exact here because the release-family flags carry no-gandvm.ccontains no__LINE__/__FILE__/assert(. The release suite log also diffs to zero lines against its pre-change baseline, andmake freestanding-check's log is byte-identical.Builds, commands lifted from
.github/workflows/ci.yml:make,CC=gcc ./build.sh,CC=clang ./build.sh,make http,make gfx,make full,make asan— all pass. No new warnings; the clang warning multiset is identical to baseline oncefile:lineis stripped (thevm.c'len' may be used uninitializedline moves4975 → 4950, exactly the 25 lines removed).Suite (
cd tests && bash run_all_tests.sh, direct exit code 0 on each): release 3364/3364, http+model 3468/3468, gfx 3405/3405, full 3504/3504, ASan+UBSan withdetect_leaks=13368/3368 with no LeakSanitizer tally line.The libpq consequence is measured, not inferred — compiling
eigenscript.calone with thefullflags minus-I/usr/include/postgresql: before,fatal error: libpq-fe.h: No such file or directory; after, clean. (make fullstill passes that flag to every TU, since it is a variant flag inMakefile:74— the win is that the core TU no longer needs it.)On the includes, rather than sampling symbols I preprocessed two synthetic TUs — with and without the two headers — and diffed their identifier sets. The 511 identifiers the headers add intersect
eigenscript.cin 18 places, all of which resolve to parameter/struct-member names inside declarations (PQexec'squery,lo_read'sbuf, …) or#-line-marker filenames; a follow-up grep for member accesses on those names finds one hit, inside a comment. So zero declared entities are used, by whole-header enumeration rather than the issue's 7-symbol table.Bugs Discovered
None filed — nothing found here is a defect in its own right.
Follow-ups / Known Limitations
vm.c:624'sextern Value g_null_singleton_external_decl; /* not used; doc only */(I could find no definition for that symbol anywhere insrc/), and the block-scope externs atvm.c:5177-5180/:5221-5223, which are the issue's item 4.eigenscript.h:1288-1291already declaresne_softmax_buf,ne_matmul_bufandjson_obj_get, whichmodel_internal.h:143-145,209re-declares. That duplication is the header-split's problem, not this PR's.make netandmake zlibwere not built. Nothing in the diff is variant-conditional and those two differ fromreleaseonly by a-D, so their objects for these TUs are identical torelease's — happy to add them to the table if you want every variant represented.Checklist
make testpasses locally — five variants plus ASan, all green, all at exit 0docs/BUILTINS.md— n/a, no new builtinsdocs/STDLIB.md— n/aGenerated by Claude Opus 5 (brief, implementation, review)