Declare the Sound namespace in a header instead of per file - #2727
Merged
Merged
Conversation
Thirty-one call sites reached Sound:: through a mangled-name extern each file wrote for itself. Those copies had already drifted -- daBmb_c.cpp and daObjPushblock_c.cpp both declared PlayLong returning u32, while the definition returns int -- so the drift is removed here too. Sound::PlaySub and Sound::ChangeMusicVolume deliberately stay on the extern spelling. Their definitions are mangled-name C symbols carrying 5Fix12IiE, and no type this tree defines mangles to that (types.h refuses to make Fix12 a template), so a namespace-qualified call cannot reach them at all. struct dActor_c is forward-declared at GLOBAL scope on purpose: inside namespace Sound it would name Sound::dActor_c, which is not the type any caller holds. The two config/ rows are a compiler renumber, not a rename. mwccarm numbers file-local entities in parse order, so adding declarations to a header shifts them in every TU that includes it -- including d_a_obj_fm_battan.cpp, which mentions none of them. tools/tu_manifest_resync.py paired both rows by position with section and size matching; addresses, sizes and evidence are untouched. Gates: 106/106 exact, mismatching 0, ROM-build analysis PASS (59 compiled). CONVERTED 2812 -> 2822. Eligible set identical at 11243. No new declaration disagreements, and 34 banked ones disappear. port_refcheck 423/423, MSVC port smoke build 12/12. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y4FxdKHG3a6hQRYRAbx29c
✅ PR validation — PassedCommitted merge introduces no reconstruction or attribution regression. Full merge validation
Byte-verified means the range carries Warnings: 2 linkcheck result(s) have unresolved relocations. Per-file link-check detailAll 60 changed file(s) compile to the ROM byte-for-byte with correct relocation targets.
The private worker commits a test merge, builds the stock ROM profile, compares every executable module, measures matched and source-built code, checks contributor lineage, and verifies affected relocations. The mod profile is opt-in and is not part of this merge gate. |
Collaborator
Author
|
Merge-tree check: Each of these PRs is gated alone, so green-alone does not imply green-merged. I built the three-way merge locally and ran the full set:
File sets are disjoint across all three pairs, so there is no textual conflict either. One thing specifically checked, since it would have been invisible otherwise: #2727 carries a 🤖 Generated with Claude Code |
This was referenced Sep 16, 2026
Collaborator
Author
Five-way merge tree gated (main + #2725 + #2726 + #2727 + #2728 + #2729)Each of these five PRs is green on its own, but this repo has produced a red Disjointness. 122 changed files across the five; all 10 pairs share zero files, Cold rebuild ( The rebuild is deliberately cold. A warm run on this same tree reported
🤖 Generated with Claude Code |
Collaborator
Author
Adversarial review — seven-PR set (#2725–#2731)Re-reviewed as a critic rather than as the author. Every check below is new work, not a 1. Relocation destinations — the hazard the byte gate cannot seeA relocated word is a wildcard: a
0 WRONG-DEST across 125 files and 61 TU functions. This also closes the cast-receiver 2. Merge tree, cold
3. Change-kind census139 files changed in the merge tree, all 4. Local-only gates, run on the merge tree (CI runs none of these on a merge tree)
5. The merged decl baseline is not a silent clobber
0 hidden. Every difference is in the safe direction. (Separately: ~84 stale banked entries 6. The MSVC port build — differential, not absolute
The other six PRs introduce zero new port link errors. What remains is the pre-existing 7. Per-PR findings
VerdictNo defect found, and no change requested. Both incomplete migrations are incomplete on The one thing this review cannot supply is the thing the review gate actually wants: I am the 🤖 Generated with Claude Code |
andrewboudreau
pushed a commit
to lunavyqo/sm64ds-decomp
that referenced
this pull request
Sep 18, 2026
The six one-function BasementWater sources and the d_a_obj_c0_water.c factory become one class TU under the cartridge's own RTTI name. ov012 0x02111450..0x02111768 is now a single `complete` delinks range, and symbols.txt drops the coined _ZTV13BasementWater alias that shared a word with _ZTV14daObjC0Water_c. Rebased onto main, which had landed changes in two of the absorbed sources. Both are carried forward rather than reverted: - _ZN13BasementWater6RenderEv.cpp (tangosdev#2728) traded the mangled _ZN18TextureTransformer6UpdateER15ModelComponents for a real member call. The absorbed body already spells it `mTextureTransformer.Update(mModel.data)` on named members, and Update is non-virtual -- _ZTV18TextureTransformer at 0x0208e7c4 holds only the destructor pair -- so this is the direct call the ROM has, not a vtable dispatch. - _ZN13BasementWater8BehaviorEv.cpp (tangosdev#2727) replaced the per-file Sound externs with Sound.h. The absorbed body had the PlaySecretSound half but still declared its own `namespace Sound` block and still called PlayLong by mangled name, so both are ported here: the TU includes Sound.h and calls Sound::PlayLong. The file comment claiming Sound.h has no PlayLong was stale and is corrected. The class rename had also rewritten historical file citations into paths that never existed -- `src/_ZN14daObjC0Water_c*.cpp` in this entry's `legacy_source` and `substitutedObjectPaths`, and in the notes/data/tu-merge-candidates.json census row. Those name the files that were absorbed, which were the BasementWater spellings; they are restored, matching how ov012/daObjC0_Switch_c.json records its own fold. config/decl-agreement-baseline.json is edited to this PR's own entries only -- 11 removed with the deleted sources, 5 added for the class TU -- rather than regenerated, so main's unrelated staleness is not laundered through here. converted-baseline.json keeps its sorted shape: 3 paths out, 3 #symbol identities in, count unchanged at 2813. rombuild -j16 --no-rom --no-cache: 8,749 enrolled sources compiled cold, 11,206 source-built functions, 11,206 reproducing, 0 mismatching, 106/106 modules exact at 100.000000%, ROM-build analysis PASS. check_decl_agreement, check_dead_references, check_src_tu and check_tubuild_conflicts --list all pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G1Gdj5fTjMsW2VjRLpXxYA
andrewboudreau
pushed a commit
to lunavyqo/sm64ds-decomp
that referenced
this pull request
Sep 18, 2026
The six one-function BasementWater sources and the d_a_obj_c0_water.c factory become one class TU under the cartridge's own RTTI name. ov012 0x02111450..0x02111768 is now a single `complete` delinks range, and symbols.txt drops the coined _ZTV13BasementWater alias that shared a word with _ZTV14daObjC0Water_c. Rebased onto main, which had landed changes in two of the absorbed sources. Both are carried forward rather than reverted: - _ZN13BasementWater6RenderEv.cpp (tangosdev#2728) traded the mangled _ZN18TextureTransformer6UpdateER15ModelComponents for a real member call. The absorbed body already spells it `mTextureTransformer.Update(mModel.data)` on named members, and Update is non-virtual -- _ZTV18TextureTransformer at 0x0208e7c4 holds only the destructor pair -- so this is the direct call the ROM has, not a vtable dispatch. - _ZN13BasementWater8BehaviorEv.cpp (tangosdev#2727) replaced the per-file Sound externs with Sound.h. The absorbed body had the PlaySecretSound half but still declared its own `namespace Sound` block and still called PlayLong by mangled name, so both are ported here: the TU includes Sound.h and calls Sound::PlayLong. The file comment claiming Sound.h has no PlayLong was stale and is corrected. The class rename had also rewritten historical file citations into paths that never existed -- `src/_ZN14daObjC0Water_c*.cpp` in this entry's `legacy_source` and `substitutedObjectPaths`, and in the notes/data/tu-merge-candidates.json census row. Those name the files that were absorbed, which were the BasementWater spellings; they are restored, matching how ov012/daObjC0_Switch_c.json records its own fold. The manifest note describing a "PlayLong seam kept below" is updated with it: there is no mangled seam here any more. config/decl-agreement-baseline.json is edited to this PR's own entries only -- 11 removed with the deleted sources, 5 added for the class TU -- rather than regenerated, so main's unrelated staleness is not laundered through here. converted-baseline.json keeps its sorted shape: 3 paths out, 3 #symbol identities in, count unchanged at 2813. rombuild -j16 --no-rom --no-cache: 8,749 enrolled sources compiled cold, 11,206 source-built functions, 11,206 reproducing, 0 mismatching, 106/106 modules exact at 100.000000%, ROM-build analysis PASS. check_decl_agreement, check_dead_references, check_src_tu and check_tubuild_conflicts --list all pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G1Gdj5fTjMsW2VjRLpXxYA
andrewboudreau
added a commit
that referenced
this pull request
Sep 18, 2026
* Cleanup ov012/daObjC0Water_c: promote BasementWater class TU The six one-function BasementWater sources and the d_a_obj_c0_water.c factory become one class TU under the cartridge's own RTTI name. ov012 0x02111450..0x02111768 is now a single `complete` delinks range, and symbols.txt drops the coined _ZTV13BasementWater alias that shared a word with _ZTV14daObjC0Water_c. Rebased onto main, which had landed changes in two of the absorbed sources. Both are carried forward rather than reverted: - _ZN13BasementWater6RenderEv.cpp (#2728) traded the mangled _ZN18TextureTransformer6UpdateER15ModelComponents for a real member call. The absorbed body already spells it `mTextureTransformer.Update(mModel.data)` on named members, and Update is non-virtual -- _ZTV18TextureTransformer at 0x0208e7c4 holds only the destructor pair -- so this is the direct call the ROM has, not a vtable dispatch. - _ZN13BasementWater8BehaviorEv.cpp (#2727) replaced the per-file Sound externs with Sound.h. The absorbed body had the PlaySecretSound half but still declared its own `namespace Sound` block and still called PlayLong by mangled name, so both are ported here: the TU includes Sound.h and calls Sound::PlayLong. The file comment claiming Sound.h has no PlayLong was stale and is corrected. The class rename had also rewritten historical file citations into paths that never existed -- `src/_ZN14daObjC0Water_c*.cpp` in this entry's `legacy_source` and `substitutedObjectPaths`, and in the notes/data/tu-merge-candidates.json census row. Those name the files that were absorbed, which were the BasementWater spellings; they are restored, matching how ov012/daObjC0_Switch_c.json records its own fold. The manifest note describing a "PlayLong seam kept below" is updated with it: there is no mangled seam here any more. config/decl-agreement-baseline.json is edited to this PR's own entries only -- 11 removed with the deleted sources, 5 added for the class TU -- rather than regenerated, so main's unrelated staleness is not laundered through here. converted-baseline.json keeps its sorted shape: 3 paths out, 3 #symbol identities in, count unchanged at 2813. rombuild -j16 --no-rom --no-cache: 8,749 enrolled sources compiled cold, 11,206 source-built functions, 11,206 reproducing, 0 mismatching, 106/106 modules exact at 100.000000%, ROM-build analysis PASS. check_decl_agreement, check_dead_references, check_src_tu and check_tubuild_conflicts --list all pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G1Gdj5fTjMsW2VjRLpXxYA * Credit the four consolidated BasementWater members to their real owners The promotion wrote all seven members of the folded TU from the factory's file-level owner instead of each function's own. Four of the seven were wrong: Behavior belongs to lunavyqo, both destructors to tangosdev, and classInit to andrewboudreau. Derived from the attribution gate's own prior-owner computation across the fold, not from git blame: run base..head, every consolidated function reports the owner it carried before the move. With these four rows the run reports 0 changed, 0 lost and all 7 consolidations preserved; without them it reports 4 changed. No path-wide override is added -- the correction is per member. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G1Gdj5fTjMsW2VjRLpXxYA --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: andrewboudreau <andrewboudreau@gmail.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
1 participant
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.
31 call sites reached
Sound::through a mangled-name extern each file wrote for itself. This replaces those copies with one set of declarations ininclude/Sound.h.The copies had already drifted:
daBmb_c.cppanddaObjPushblock_c.cppboth declaredPlayLongreturningu32, while the definition atsrc/_ZN5Sound8PlayLongEjjjRK7Vector3s.cpp:61returnsint. Adopting the header made that an ambiguous overload, so both stale declarations are removed here. Worth noting for anyone doing the same elsewhere: only the fullrombuildcatches this class of break — it reds files the PR never touches, and a per-filebuild_pin.verifynever sees it.Two functions deliberately left alone
Sound::PlaySubandSound::ChangeMusicVolumekeep the extern spelling. Their definitions are mangled-name C symbols carrying5Fix12IiE, and no type this tree defines mangles to that —types.hrefuses to makeFix12a template, and a by-valueFix12<int>parameter would contradict the bytes anyway. A namespace-qualified call cannot reach them, so declaring them just breaks the link. The header says so, in place.struct dActor_cis forward-declared at global scope on purpose. Insidenamespace Soundit would declareSound::dActor_c, which is not the type any caller holds.The two
config/rows are a renumber, not a renamed_a_obj_fm_battan.cppmentions none of the added declarations and is untouched by this PR, but its intact-TU prep failed withbinding rewrite symbol @348 has 0 entries. mwccarm numbers file-local entities in parse order, so adding declarations to a shared header renumbers them in every TU that includes it.tools/tu_manifest_resync.pypaired both rows by position with section and size agreeing entry-for-entry; only the numbers move — addresses, sizes and evidence are unchanged.Gates
rombuild --no-rom106/106 exact,mismatching: 0,ROM-build analysis: PASS, 59 compiledtiers_ratchet --checkeligible.pycheck_decl_agreementport_refcheckThe decl-agreement baseline is not updated here — the 34 vanished rows are a pure shrink and the gate does not fail on them, so this avoids adding churn to a known rebase chokepoint.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Y4FxdKHG3a6hQRYRAbx29c