chore(core,vm): drop the two dead extension includes and 29 stale externs (#744, items 1-2) - #811
Open
Nitjsefnie wants to merge 1 commit into
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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)