Skip to content

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
InauguralSystems:mainfrom
Nitjsefnie-OSC:chore/drop-dead-includes-stale-externs-744
Open

chore(core,vm): drop the two dead extension includes and 29 stale externs (#744, items 1-2)#811
Nitjsefnie wants to merge 1 commit into
InauguralSystems:mainfrom
Nitjsefnie-OSC:chore/drop-dead-includes-stale-externs-744

Conversation

@Nitjsefnie

Copy link
Copy Markdown
Contributor

What does this PR do?

Takes the two cheapest items of #744 — the dead extension includes in eigenscript.c and the stale extern block in vm.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.h umbrella are untouched, so #744 stays open.

Changes

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_dict and env_get_assign_count are called from vm.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 — are static inline definitions there (:976, :984, :955), so the extern was 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 claimed observer_ensure_fresh came from eval.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}.o from the unmodified merge-base, applied the change, rebuilt, and cmp'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 -g and vm.c contains no __LINE__/__FILE__/assert(. The release suite log also diffs to zero lines against its pre-change baseline, and make 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 once file:line is stripped (the vm.c 'len' may be used uninitialized line moves 4975 → 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 with detect_leaks=1 3368/3368 with no LeakSanitizer tally line.

The libpq consequence is measured, not inferred — compiling eigenscript.c alone with the full flags minus -I/usr/include/postgresql: before, fatal error: libpq-fe.h: No such file or directory; after, clean. (make full still passes that flag to every TU, since it is a variant flag in Makefile: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.c in 18 places, all of which resolve to parameter/struct-member names inside declarations (PQexec's query, lo_read's buf, …) 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

  • The three surviving externs want a header declaration; that is part of Layering is convention, not structure: no per-layer headers, eigenscript.h is a 1253-line umbrella, eigenscript.c carries two dead extension includes, vm.c has 32 stale externs #744's header work.
  • Two related items left alone as out of scope: vm.c:624's extern Value g_null_singleton_external_decl; /* not used; doc only */ (I could find no definition for that symbol anywhere in src/), and the block-scope externs at vm.c:5177-5180 / :5221-5223, which are the issue's item 4.
  • Pre-existing and unaffected: eigenscript.h:1288-1291 already declares ne_softmax_buf, ne_matmul_buf and json_obj_get, which model_internal.h:143-145,209 re-declares. That duplication is the header-split's problem, not this PR's.
  • make net and make zlib were not built. Nothing in the diff is variant-conditional and those two differ from release only by a -D, so their objects for these TUs are identical to release's — happy to add them to the table if you want every variant represented.

Checklist

  • make test passes locally — five variants plus ASan, all green, all at exit 0
  • New builtins have signature comments and docs in docs/BUILTINS.md — n/a, no new builtins
  • New library functions follow conventions in docs/STDLIB.md — n/a
  • New examples have a comment header explaining what they demonstrate — n/a
  • CHANGELOG.md updated (if user-facing change) — n/a, declaration-only with byte-identical objects; no user-visible change to record

Generated by Claude Opus 5 (brief, implementation, review)

…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant