Skip to content

refactor(sysrap): replace ssincos with smath wrapper#403

Open
plexoos wants to merge 2 commits into
mainfrom
refactor-sincos
Open

refactor(sysrap): replace ssincos with smath wrapper#403
plexoos wants to merge 2 commits into
mainfrom
refactor-sincos

Conversation

@plexoos

@plexoos plexoos commented Jul 6, 2026

Copy link
Copy Markdown
Member
  • Add smath::sincos as a pair-returning floating-point wrapper around std::sin and std::cos.
  • Replace ssincos call sites with smath::sincos structured bindings and remove the obsolete ssincos.h installed header.
  • Move sin/cos coverage into smath_test, register smath_test with CTest, and remove the standalone ssincosTest.cc.
  • Make smath_test run all smath.h helper checks directly, with explicit failure reporting for sincos, count_nibbles, rotateUz, and erfcinvf.

- Add smath::sincos as a pair-returning floating-point wrapper around std::sin and std::cos.
- Replace ssincos call sites with smath::sincos structured bindings and remove the obsolete ssincos.h installed header.
- Move sin/cos coverage into smath_test, register smath_test with CTest, and remove the standalone ssincosTest.cc.
- Make smath_test run all smath.h helper checks directly, with explicit failure reporting for sincos, count_nibbles, rotateUz, and erfcinvf.
Copilot AI review requested due to automatic review settings July 6, 2026 23:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors trigonometric helper usage in sysrap by introducing smath::sincos (pair-returning wrapper around std::sin/std::cos), migrating existing call sites off the legacy ssincos helper, and consolidating test coverage into smath_test registered with CTest.

Changes:

  • Add smath::sincos(T) in sysrap/smath.h and replace ssincos(...) call sites with structured bindings.
  • Remove the obsolete installed header sysrap/ssincos.h and the standalone test sysrap/tests/ssincosTest.cc.
  • Expand sysrap/tests/smath_test.cc to directly exercise sincos, count_nibbles, rotateUz, and erfcinvf, and register smath_test with CTest via sysrap/tests/CMakeLists.txt.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
sysrap/tests/ssincosTest.cc Removes obsolete standalone sin/cos test program.
sysrap/tests/smath_test.cc Adds direct failure-reporting checks for smath helpers, including new sincos.
sysrap/tests/SEventTest.cc Removes unused ssincos.h include after migration.
sysrap/tests/CMakeLists.txt Registers smath_test.cc in the SysRap test suite (CTest).
sysrap/ssincos.h Removes legacy installed header providing ssincos.
sysrap/smath.h Adds smath::sincos wrapper and required standard headers.
sysrap/SFrameGenstep.cc Replaces ssincos usage with smath::sincos structured bindings.
CSG/tests/CSGIntersectComparisonTest.cc Replaces ssincos usage with smath::sincos structured bindings.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@plexoos plexoos self-assigned this Jul 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants