refactor(sysrap): replace ssincos with smath wrapper#403
Open
plexoos wants to merge 2 commits into
Open
Conversation
plexoos
commented
Jul 6, 2026
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.
Contributor
There was a problem hiding this comment.
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)insysrap/smath.hand replacessincos(...)call sites with structured bindings. - Remove the obsolete installed header
sysrap/ssincos.hand the standalone testsysrap/tests/ssincosTest.cc. - Expand
sysrap/tests/smath_test.ccto directly exercisesincos,count_nibbles,rotateUz, anderfcinvf, and registersmath_testwith CTest viasysrap/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.
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
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.