all: Enforce weak / global functions#269
Conversation
c6cecd4 to
683f3ae
Compare
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 reviewed 8 files and all commit messages, and made 3 comments.
Reviewable status: 8 of 10 files reviewed, 3 unresolved discussions (waiting on german77).
modules/src/devenv/seadFontMgr.cpp at r1 (raw file):
File is empty now => delete it (and remove from CMakeLists.txt)
modules/src/math/seadMathCalcCommon.cpp at r1 (raw file):
same here
modules/src/prim/seadStringBuilder.cpp at r1 (raw file):
This seems wrong, and ... to my knowledge, is wrong.
Checking <char>::copy and <char16_t>::copy within Labo, which has SEAD_ASSERTs enabled, we can see that for both instantiations, the str must not be null message is generated in line 42 - so it must be coming from the same function implementation.
Please revert these ones (StringBuilder and MathCalcCommon) which introduce duplicate code - hopefully we'll figure out something in time, otherwise we'll have to come up with a "blacklist" to ignore specific issues, but introducing that much duplicate code is just wrong.
german77
left a comment
There was a problem hiding this comment.
@german77 made 1 comment.
Reviewable status: 8 of 10 files reviewed, 3 unresolved discussions (waiting on MonsterDruide1).
modules/src/prim/seadStringBuilder.cpp at r1 (raw file):
Previously, MonsterDruide1 wrote…
This seems wrong, and ... to my knowledge, is wrong.
Checking<char>::copyand<char16_t>::copywithin Labo, which hasSEAD_ASSERTs enabled, we can see that for both instantiations, thestr must not be nullmessage is generated in line42- so it must be coming from the same function implementation.Please revert these ones (StringBuilder and MathCalcCommon) which introduce duplicate code - hopefully we'll figure out something in time, otherwise we'll have to come up with a "blacklist" to ignore specific issues, but introducing that much duplicate code is just wrong.
I kind of agree that this isn't the correct way to do it since it makes the code awful. Thanks for the assert suggestion. I will try to find another solution and if it takes too long I will fully rever these changes.
german77
left a comment
There was a problem hiding this comment.
@german77 made 3 comments.
Reviewable status: 7 of 12 files reviewed, 3 unresolved discussions (waiting on MonsterDruide1).
modules/src/devenv/seadFontMgr.cpp at r1 (raw file):
Previously, MonsterDruide1 wrote…
File is empty now => delete it (and remove from
CMakeLists.txt)
Done.
modules/src/math/seadMathCalcCommon.cpp at r1 (raw file):
Previously, MonsterDruide1 wrote…
same here
Done. Sadly gcd now has a regswap
modules/src/prim/seadStringBuilder.cpp at r1 (raw file):
Previously, german77 (Narr the Reg) wrote…
I kind of agree that this isn't the correct way to do it since it makes the code awful. Thanks for the assert suggestion. I will try to find another solution and if it takes too long I will fully rever these changes.
Done. Now every function has an Impl_ equivalent. I made sure no unwanted symbols are generated in this conversion.
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 reviewed 5 files and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on german77).
modules/src/math/seadMathCalcCommon.cpp at r1 (raw file):
Previously, german77 (Narr the Reg) wrote…
Done. Sadly gcd now has a regswap
Better than code duplication, in my opinion - maybe we'll find a solution eventually.
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on german77).
a discussion (no related file):
The workflow is exploding with more error messages than I would expect, can you look into why that happens?
german77
left a comment
There was a problem hiding this comment.
@german77 made 1 comment.
Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion (waiting on MonsterDruide1).
a discussion (no related file):
Previously, MonsterDruide1 wrote…
The workflow is exploding with more error messages than I would expect, can you look into why that happens?
Which one to be specific? Not everything is a mismatch. There are new matches as well. Botw is missing LogicalFrameBuffer but that's normal I checked their source and they don't use it yet. SMO reported LogicalFrameBuffer, FontBase and gcd errors but those are expected as well since I broke gcd and the other aren't used yet.
Sadly the rebuild used our new check tool that reports more errors so SMO is bloated with additional errors unrelated to this PR now.
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on german77).
a discussion (no related file):
Previously, german77 (Narr the Reg) wrote…
Which one to be specific? Not everything is a mismatch. There are new matches as well. Botw is missing LogicalFrameBuffer but that's normal I checked their source and they don't use it yet. SMO reported LogicalFrameBuffer, FontBase and gcd errors but those are expected as well since I broke gcd and the other aren't used yet.
Sadly the rebuild used our new check tool that reports more errors so SMO is bloated with additional errors unrelated to this PR now.
thread '<unnamed>' panicked at src/tools/check.rs:521:88:called `Result::unwrap()` on an `Err` value: unknown function: _ZN4sead8FontBaseD2Ev
Failing on sead::FontBase is fine/expected, but panicked seems wrong. I can't pinpoint why that happens though - let's just wait for a bit for the basic fixes with the new check to be merged, then check again if it still panic!s here.
This is a continuation of #262. Fully eliminates all weak / global mismatches from sead. Unlike the previous PR I call it an aggressive approach since it will break some matches and duplicate code. The lost matches will be recovered once the object is used.
This change is