Add snitch-based unit tests for PSYQo#2008
Conversation
Vendor snitch (single-header C++20 test framework, freestanding config) and arith64 (64-bit division builtins for MIPS). 111 test cases covering FixedPoint, Vector, Matrix, SoftMath, Trig, MSF, Adler32, Bezier, and GPU primitives. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughIntegrates the Snitch test framework and vendored arith64 into PSYQo, adds build rules and recursive make targets, introduces a PSYQo test runner with many new unit tests, adds third-party sources and licenses, and provides a weak C++ ABI hook for out_of_range formatting. Changes
Sequence Diagram(s)sequenceDiagram
participant HostTest as Host Test Runner
participant Emulator as pcsx Emulator
participant Guest as PSYQo Test Executable
participant Snitch as Snitch Framework
rect rgba(200,200,255,0.5)
HostTest->>Emulator: launch ( -no-ui -testmode, PSYQo .ps-exe )
end
rect rgba(200,255,200,0.5)
Emulator->>Guest: load & start PSYQo test exe
Guest->>Snitch: snitch::tests.run_tests("psyqo")
Snitch->>Guest: call console_print / print callbacks
Guest->>Emulator: syscall_putchar / ramsyscall_printf output
end
rect rgba(255,200,200,0.5)
Guest->>Emulator: exit(status)
Emulator->>HostTest: return exit code
HostTest->>HostTest: ASSERT exit code == 0
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tests/pcsxrunner/psyqo.cc (1)
23-35: Coverage is great; consider extracting shared invocation args to reduce drift.
Both tests are correct, but a small helper would keep future flag/path edits in one place.Refactor sketch
+static int run_psyqo(const char* cpu_mode_flag) { + MainInvoker invoker("-no-ui", "-run", "-bios", "src/mips/openbios/openbios.bin", "-testmode", cpu_mode_flag, + "-loadexe", "src/mips/tests/psyqo/psyqo-tests.ps-exe"); + return invoker.invoke(); +} + TEST(PSYQo, Interpreter) { - MainInvoker invoker("-no-ui", "-run", "-bios", "src/mips/openbios/openbios.bin", "-testmode", "-interpreter", - "-loadexe", "src/mips/tests/psyqo/psyqo-tests.ps-exe"); - int ret = invoker.invoke(); + int ret = run_psyqo("-interpreter"); EXPECT_EQ(ret, 0); } TEST(PSYQo, Dynarec) { - MainInvoker invoker("-no-ui", "-run", "-bios", "src/mips/openbios/openbios.bin", "-testmode", "-dynarec", - "-loadexe", "src/mips/tests/psyqo/psyqo-tests.ps-exe"); - int ret = invoker.invoke(); + int ret = run_psyqo("-dynarec"); EXPECT_EQ(ret, 0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/pcsxrunner/psyqo.cc` around lines 23 - 35, Extract the common invocation arguments used in both TESTs into a shared helper to reduce duplication and drift: create a function (e.g., buildPsyqoInvoker or makePsyqoArgs) that returns a configured MainInvoker or a reusable arg list, move the shared flags ("-no-ui","-run","-bios","src/mips/openbios/openbios.bin","-testmode","-loadexe","src/mips/tests/psyqo/psyqo-tests.ps-exe") into that helper, and have the TEST(PSYQo, Interpreter) and TEST(PSYQo, Dynarec) call it then only add the differing "-interpreter" or "-dynarec" flag before invoking invoke(); update tests to use the helper so future path/flag edits are done in one place.src/mips/tests/psyqo/test-vector.cpp (2)
72-154: Optional: reduce repeated Vec3 setup in arithmetic tests.The repeated
x/y/zinitialization blocks make this section harder to maintain; a small helper can keep cases concise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mips/tests/psyqo/test-vector.cpp` around lines 72 - 154, Tests repeatedly construct and initialize Vec3 fields across many TEST_CASEs; refactor by adding a small helper function (e.g., makeVec3 or Vec3::fromDoubles) used by the tests to create Vec3 instances with given x,y,z to reduce duplication. Update tests such as "Vec3 addition", "Vec3 subtraction", "Vec3 scalar multiplication", "Vec3 FixedPoint multiplication", "Vec3 scalar division", "Vec3 negation", and "Vec3 compound assignment" to call the helper instead of manually setting a.x/a.y/a.z and b.x/b.y/b.z.
17-26: Expandoperator[]coverage for all Vec3 indices.This test validates
operator[]only forLine 25index0; add indices1and2to cover the full accessor path.Suggested test extension
TEST_CASE("Vec3 element access") { Vec3 v; v.x = 1.0; v.y = 2.0; v.z = 3.0; REQUIRE(v.get(0).raw() == v.x.raw()); REQUIRE(v.get(1).raw() == v.y.raw()); REQUIRE(v.get(2).raw() == v.z.raw()); REQUIRE(v[0].raw() == v.x.raw()); + REQUIRE(v[1].raw() == v.y.raw()); + REQUIRE(v[2].raw() == v.z.raw()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mips/tests/psyqo/test-vector.cpp` around lines 17 - 26, The test only checks Vec3::operator[] for index 0, leaving indices 1 and 2 untested; update the TEST_CASE "Vec3 element access" to also assert v[1].raw() == v.y.raw() and v[2].raw() == v.z.raw(), mirroring the existing REQUIREs that compare get(1) and get(2) to y and z, so both Vec3::get and Vec3::operator[] accessors are fully covered.src/mips/psyqo/snitch.mk (1)
1-16: Refine include-guard scope and remove unnecessary phony designation from archive target.The current guard at line 1 gates the entire integration block. If
SNITCHDIRis pre-set before including this file, theifndefcheck fails andLIBRARIES,CPPFLAGS, and build rules are skipped entirely. Additionally, marking$(SNITCHDIR)libsnitch.aas phony at line 15 forces unconditional rebuilds; since the nestedmakein the snitch subdirectory has its own dependency tracking, removing the phony designation lets Make rebuild only when necessary.Suggested Makefile adjustment
ifndef SNITCHDIR SNITCHDIR := $(dir $(abspath $(lastword $(MAKEFILE_LIST))))snitch/ +endif LIBRARIES += $(SNITCHDIR)libsnitch.a CPPFLAGS += -I$(SNITCHDIR)../../../../third_party/snitch include $(SNITCHDIR)../../psyqo/psyqo.mk $(SNITCHDIR)libsnitch.a: $(MAKE) -C $(SNITCHDIR) BUILD=$(BUILD) clean:: $(MAKE) -C $(SNITCHDIR) clean -.PHONY: clean $(SNITCHDIR)libsnitch.a -endif +.PHONY: clean🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mips/psyqo/snitch.mk` around lines 1 - 16, The guard currently skips the entire integration if SNITCHDIR is preset; instead only guard the default assignment by replacing the top ifndef ... endif to wrap just "SNITCHDIR := $(dir $(abspath $(lastword $(MAKEFILE_LIST))))snitch/" (leave the rest of the file unconditional so LIBRARIES, CPPFLAGS, include and rules are always evaluated), and remove "$(SNITCHDIR)libsnitch.a" from the .PHONY list so the archive target is not treated as phony (keep "clean" phony). Ensure references to SNITCHDIR, LIBRARIES, CPPFLAGS, the include line for ../../psyqo/psyqo.mk, and the make invocation target $(SNITCHDIR)libsnitch.a remain unchanged otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mips/tests/psyqo/test-soft-math.cpp`:
- Around line 12-35: Add a small helper that asserts all nine elements of a 3x3
matrix equal the identity (1 on [0,0],[1,1],[2,2], 0 elsewhere) and use it in
each TEST_CASE instead of only checking diagonals; locate uses of
SoftMath::generateRotationMatrix33(...) and the matrix accessors m.vs[i].x/.y/.z
(and .raw()/.integer()) and replace the repeated assertion blocks in the X/Y/Z
zero-angle tests with a single call to that helper to verify the full matrix for
SoftMath::Axis::X, SoftMath::Axis::Y, and SoftMath::Axis::Z cases.
In `@src/mips/tests/psyqo/test-trigonometry.cpp`:
- Around line 17-20: The test uses exact equality for trig.cos(0.5_pi) which is
brittle; update the assertion in the TEST_CASE that calls trig.cos (and checks
c.raw()) to use a tolerance-based comparison (e.g., Catch2's Approx or an
absolute epsilon) instead of REQUIRE(c.raw() == 0), and pick a small tolerance
like 1e-6 to allow floating-point quantization error.
In `@third_party/arith64/arith64.c`:
- Around line 158-160: The code currently attempts to force a divide-by-zero
exception using undefined behavior ("volatile char x = 0; x = 1 / x;"); replace
that UB with an explicit trap primitive: remove the volatile/divide expression
and call __builtin_trap() (or an equivalent explicit trap like
abort()/raise(SIGFPE) if portability constraints dictate) at the divide-by-zero
handling site where variable b is checked, so the runtime helper
deterministically signals a trap instead of relying on undefined behavior.
---
Nitpick comments:
In `@src/mips/psyqo/snitch.mk`:
- Around line 1-16: The guard currently skips the entire integration if
SNITCHDIR is preset; instead only guard the default assignment by replacing the
top ifndef ... endif to wrap just "SNITCHDIR := $(dir $(abspath $(lastword
$(MAKEFILE_LIST))))snitch/" (leave the rest of the file unconditional so
LIBRARIES, CPPFLAGS, include and rules are always evaluated), and remove
"$(SNITCHDIR)libsnitch.a" from the .PHONY list so the archive target is not
treated as phony (keep "clean" phony). Ensure references to SNITCHDIR,
LIBRARIES, CPPFLAGS, the include line for ../../psyqo/psyqo.mk, and the make
invocation target $(SNITCHDIR)libsnitch.a remain unchanged otherwise.
In `@src/mips/tests/psyqo/test-vector.cpp`:
- Around line 72-154: Tests repeatedly construct and initialize Vec3 fields
across many TEST_CASEs; refactor by adding a small helper function (e.g.,
makeVec3 or Vec3::fromDoubles) used by the tests to create Vec3 instances with
given x,y,z to reduce duplication. Update tests such as "Vec3 addition", "Vec3
subtraction", "Vec3 scalar multiplication", "Vec3 FixedPoint multiplication",
"Vec3 scalar division", "Vec3 negation", and "Vec3 compound assignment" to call
the helper instead of manually setting a.x/a.y/a.z and b.x/b.y/b.z.
- Around line 17-26: The test only checks Vec3::operator[] for index 0, leaving
indices 1 and 2 untested; update the TEST_CASE "Vec3 element access" to also
assert v[1].raw() == v.y.raw() and v[2].raw() == v.z.raw(), mirroring the
existing REQUIREs that compare get(1) and get(2) to y and z, so both Vec3::get
and Vec3::operator[] accessors are fully covered.
In `@tests/pcsxrunner/psyqo.cc`:
- Around line 23-35: Extract the common invocation arguments used in both TESTs
into a shared helper to reduce duplication and drift: create a function (e.g.,
buildPsyqoInvoker or makePsyqoArgs) that returns a configured MainInvoker or a
reusable arg list, move the shared flags
("-no-ui","-run","-bios","src/mips/openbios/openbios.bin","-testmode","-loadexe","src/mips/tests/psyqo/psyqo-tests.ps-exe")
into that helper, and have the TEST(PSYQo, Interpreter) and TEST(PSYQo, Dynarec)
call it then only add the differing "-interpreter" or "-dynarec" flag before
invoking invoke(); update tests to use the helper so future path/flag edits are
done in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e2d7529a-0de5-4cec-a219-2a445569beaf
📒 Files selected for processing (20)
LICENSES.mdsrc/mips/psyqo/snitch.mksrc/mips/psyqo/snitch/Makefilesrc/mips/psyqo/snitch/snitch-impl.cppsrc/mips/tests/Makefilesrc/mips/tests/psyqo/Makefilesrc/mips/tests/psyqo/psyqo-tests.cppsrc/mips/tests/psyqo/test-adler32.cppsrc/mips/tests/psyqo/test-bezier.cppsrc/mips/tests/psyqo/test-fixed-point.cppsrc/mips/tests/psyqo/test-msf.cppsrc/mips/tests/psyqo/test-primitives.cppsrc/mips/tests/psyqo/test-soft-math.cppsrc/mips/tests/psyqo/test-trigonometry.cppsrc/mips/tests/psyqo/test-vector.cpptests/pcsxrunner/psyqo.ccthird_party/arith64/LICENSEthird_party/arith64/arith64.cthird_party/snitch/LICENSEthird_party/snitch/snitch_all.hpp
| TEST_CASE("cos(pi/2) = 0") { | ||
| auto c = trig.cos(0.5_pi); | ||
| REQUIRE(c.raw() == 0); | ||
| } |
There was a problem hiding this comment.
Use tolerance for cos(pi/2) to avoid brittle exact-zero checks.
Line 19 uses exact equality while nearby near-zero trig cases already tolerate quantization error.
Robustness patch
TEST_CASE("cos(pi/2) = 0") {
auto c = trig.cos(0.5_pi);
- REQUIRE(c.raw() == 0);
+ auto diff = c.raw();
+ REQUIRE(diff >= -2);
+ REQUIRE(diff <= 2);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TEST_CASE("cos(pi/2) = 0") { | |
| auto c = trig.cos(0.5_pi); | |
| REQUIRE(c.raw() == 0); | |
| } | |
| TEST_CASE("cos(pi/2) = 0") { | |
| auto c = trig.cos(0.5_pi); | |
| auto diff = c.raw(); | |
| REQUIRE(diff >= -2); | |
| REQUIRE(diff <= 2); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mips/tests/psyqo/test-trigonometry.cpp` around lines 17 - 20, The test
uses exact equality for trig.cos(0.5_pi) which is brittle; update the assertion
in the TEST_CASE that calls trig.cos (and checks c.raw()) to use a
tolerance-based comparison (e.g., Catch2's Approx or an absolute epsilon)
instead of REQUIRE(c.raw() == 0), and pick a small tolerance like 1e-6 to allow
floating-point quantization error.
| if (b == 0) // divide by 0 | ||
| { | ||
| volatile char x = 0; x = 1 / x; // force an exception |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file and verify it exists
fd arith64.c | head -20Repository: grumpycoders/pcsx-redux
Length of output: 96
🏁 Script executed:
# Read the relevant section of the file to see the actual code
if [ -f "third_party/arith64/arith64.c" ]; then
wc -l "third_party/arith64/arith64.c"
echo "=== Lines 150-170 ==="
sed -n '150,170p' "third_party/arith64/arith64.c"
fiRepository: grumpycoders/pcsx-redux
Length of output: 1011
🏁 Script executed:
# Get broader context around the divide-by-zero handling
sed -n '130,180p' "third_party/arith64/arith64.c"Repository: grumpycoders/pcsx-redux
Length of output: 2234
🏁 Script executed:
# Check the file header and comments about intended approach
head -50 "third_party/arith64/arith64.c"Repository: grumpycoders/pcsx-redux
Length of output: 1570
🏁 Script executed:
# Search for other error/exception handling patterns in the file
rg -n "trap|abort|exception|divide" "third_party/arith64/arith64.c" -iRepository: grumpycoders/pcsx-redux
Length of output: 274
Don't use undefined behavior to signal divide-by-zero.
Division by zero in C is undefined behavior. While the volatile qualifier prevents compile-time optimization, it doesn't guarantee trap behavior at runtime; compilers may optimize the expression away entirely or behave unpredictably across platforms. In a compiler runtime helper like this, use an explicit trap primitive instead.
Replace the UB attempt with __builtin_trap():
Recommended fix
if (b == 0) // divide by 0
{
- volatile char x = 0; x = 1 / x; // force an exception
+ __builtin_trap();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (b == 0) // divide by 0 | |
| { | |
| volatile char x = 0; x = 1 / x; // force an exception | |
| if (b == 0) // divide by 0 | |
| { | |
| __builtin_trap(); |
🧰 Tools
🪛 Cppcheck (2.20.0)
[error] 160-160: Division by zero.
(zerodiv)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@third_party/arith64/arith64.c` around lines 158 - 160, The code currently
attempts to force a divide-by-zero exception using undefined behavior ("volatile
char x = 0; x = 1 / x;"); replace that UB with an explicit trap primitive:
remove the volatile/divide expression and call __builtin_trap() (or an
equivalent explicit trap like abort()/raise(SIGFPE) if portability constraints
dictate) at the divide-by-zero handling site where variable b is checked, so the
runtime helper deterministically signals a trap instead of relying on undefined
behavior.
|
@copilot resolve the merge conflicts in this pull request |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/mips/tests/psyqo/test-primitives.cpp (2)
51-57: Endianness-dependent assertion — document or guard.
packed == 0x56781234uassumes little-endian layout of theVertexunion. That's always true for the PS1 target, but if this suite is ever compiled for a big-endian host (e.g., via the VS runner path it is linked into elsewhere), the assertion will flip. Consider a brief comment or astatic_assert(std::endian::native == std::endian::little)to make the dependency explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mips/tests/psyqo/test-primitives.cpp` around lines 51 - 57, The test "Vertex packed representation" assumes little-endian layout for the Vertex union (checking v.packed == 0x56781234u); make this explicit or guard it: add a compile-time assertion (e.g., static_assert on std::endian::native == std::endian::little) before the test or wrap the REQUIRE in a runtime/endian conditional, or at minimum add a comment clarifying the endianness dependency; reference the Vertex type and its packed member and the TEST_CASE name when editing.
65-75: Minor: initializeRectbefore partial field writes.
Rect r;leavespos/sizeindeterminate; the test only writes the specific union aliases it checks. That's fine under the currentVertexlayout, but an explicitRect r{};removes any doubt and avoids UB if someone later adds a non-aliased field.- Rect r; + Rect r{};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mips/tests/psyqo/test-primitives.cpp` around lines 65 - 75, The test "Rect isEmpty" currently declares an uninitialized Rect with `Rect r;` which leaves pos/size indeterminate; change the declaration to value-initialize the struct (e.g., `Rect r{}`) in the TEST_CASE so all fields start zeroed, keeping the subsequent partial writes and REQUIRE checks the same; update the Rect declaration in the TEST_CASE to use brace initialization to avoid potential UB if Rect gains non-aliased fields later.src/mips/tests/psyqo/psyqo-tests.cpp (1)
36-40: Optional: widen viaunsigned charto avoid sign-extension of high-byte characters.
charon this toolchain is signed, so bytes ≥ 0x80 in the UTF-8/binary output would be sign-extended into theintargument ofsyscall_putchar. The BIOS putchar only uses the low byte so it works in practice, but the intent is clearer with an explicit cast.Nit
- for (char c : message) { - syscall_putchar(c); + for (char c : message) { + syscall_putchar(static_cast<unsigned char>(c)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mips/tests/psyqo/psyqo-tests.cpp` around lines 36 - 40, psyqo_console_print currently iterates over std::string_view with signed char which can sign-extend bytes >= 0x80 when passed to syscall_putchar; update the loop to treat each byte as unsigned (e.g., iterate over unsigned char or cast each character to unsigned char before calling syscall_putchar) so syscall_putchar receives the intended low byte value; target the psyqo_console_print function and the syscall_putchar call when making this change.src/mips/tests/psyqo/test-soft-math.cpp (1)
215-221: Consider asserting off-diagonals after scaling too.
scaleMatrix33is only verified along the diagonal of the identity input, so a buggy implementation that leaves non-zero off-diagonal scratch inm.vs[i].get(j)(i≠j) would still pass. Using the existingrequireIdentityMatrix33-style full-matrix check (scaled by 2) would close that gap — consistent with the earlier refactor that eliminated the zero-angle diagonal-only checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mips/tests/psyqo/test-soft-math.cpp` around lines 215 - 221, The test only checks diagonal elements after calling SoftMath::scaleMatrix33 on the identity rotation matrix; update the TEST_CASE("Matrix scale") to also verify off-diagonal entries are still zero by either calling the existing requireIdentityMatrix33-style helper scaled by FixedPoint<>(2.0) or adding explicit REQUIREs for m.vs[i].get(j).integer() == 0 for all i != j so scaleMatrix33 cannot leave stale non-zero off-diagonals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/mips/tests/psyqo/psyqo-tests.cpp`:
- Around line 36-40: psyqo_console_print currently iterates over
std::string_view with signed char which can sign-extend bytes >= 0x80 when
passed to syscall_putchar; update the loop to treat each byte as unsigned (e.g.,
iterate over unsigned char or cast each character to unsigned char before
calling syscall_putchar) so syscall_putchar receives the intended low byte
value; target the psyqo_console_print function and the syscall_putchar call when
making this change.
In `@src/mips/tests/psyqo/test-primitives.cpp`:
- Around line 51-57: The test "Vertex packed representation" assumes
little-endian layout for the Vertex union (checking v.packed == 0x56781234u);
make this explicit or guard it: add a compile-time assertion (e.g.,
static_assert on std::endian::native == std::endian::little) before the test or
wrap the REQUIRE in a runtime/endian conditional, or at minimum add a comment
clarifying the endianness dependency; reference the Vertex type and its packed
member and the TEST_CASE name when editing.
- Around line 65-75: The test "Rect isEmpty" currently declares an uninitialized
Rect with `Rect r;` which leaves pos/size indeterminate; change the declaration
to value-initialize the struct (e.g., `Rect r{}`) in the TEST_CASE so all fields
start zeroed, keeping the subsequent partial writes and REQUIRE checks the same;
update the Rect declaration in the TEST_CASE to use brace initialization to
avoid potential UB if Rect gains non-aliased fields later.
In `@src/mips/tests/psyqo/test-soft-math.cpp`:
- Around line 215-221: The test only checks diagonal elements after calling
SoftMath::scaleMatrix33 on the identity rotation matrix; update the
TEST_CASE("Matrix scale") to also verify off-diagonal entries are still zero by
either calling the existing requireIdentityMatrix33-style helper scaled by
FixedPoint<>(2.0) or adding explicit REQUIREs for m.vs[i].get(j).integer() == 0
for all i != j so scaleMatrix33 cannot leave stale non-zero off-diagonals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9e4630d4-255d-446f-8671-c6b7a1486ba8
📒 Files selected for processing (11)
src/mips/tests/psyqo/psyqo-tests.cppsrc/mips/tests/psyqo/test-adler32.cppsrc/mips/tests/psyqo/test-bezier.cppsrc/mips/tests/psyqo/test-fixed-point.cppsrc/mips/tests/psyqo/test-msf.cppsrc/mips/tests/psyqo/test-primitives.cppsrc/mips/tests/psyqo/test-soft-math.cppsrc/mips/tests/psyqo/test-trigonometry.cppsrc/mips/tests/psyqo/test-vector.cppvsprojects/tests/pcsxrunner/pcsxrunner.vcxprojvsprojects/tests/pcsxrunner/pcsxrunner.vcxproj.filters
✅ Files skipped from review due to trivial changes (1)
- vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj.filters
nicolasnoble
force-pushed
the
snitch-integration
branch
from
faf677d to
bd7eb31
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mips/common/crt0/cxxglue.c`:
- Around line 234-236: The function `_ZSt24__throw_out_of_range_fmtPKcz` is
missing an explicit return type; add the correct return type `void` to its
declaration/definition so it reads `void
_ZSt24__throw_out_of_range_fmtPKcz(const char* format, ...) { abort(); }`
ensuring the mangled symbol matches `std::__throw_out_of_range_fmt(const char*,
...)` and eliminates implicit-int warnings/errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ccbdb4e6-bdd6-44c7-9aa1-f81902487d94
📒 Files selected for processing (2)
src/mips/common/crt0/cxxglue.csrc/mips/tests/psyqo/psyqo-tests.cpp
nicolasnoble
force-pushed
the
snitch-integration
branch
from
bd7eb31 to
45ac260
Compare
1 participant
Vendor snitch (single-header C++20 test framework, freestanding config) and arith64 (64-bit division builtins for MIPS). 111 test cases covering FixedPoint, Vector, Matrix, SoftMath, Trig, MSF, Adler32, Bezier, and GPU primitives.