Fixing casting issue in fixed-point.hh#1941
Conversation
The `integer()` overload with a different type had inconsistent codepath between signed and unsigned, resulting in precision problems.
WalkthroughThe change updates the return expression in the templated Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant FixedPoint
Caller->>FixedPoint: integer<U>()
Note right of FixedPoint: Compute (value + scale/2) / scale
FixedPoint-->>Caller: Return result as type U
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a precision inconsistency in the integer() overload for unsigned types by performing the division before casting, matching the signed path’s behavior.
- Adjusted the return expression to divide before converting to
Ufor unsigned rounding. - Ensures consistent rounding logic between signed and unsigned overloads.
Comments suppressed due to low confidence (2)
src/mips/psyqo/fixed-point.hh:175
- There should be unit tests covering edge cases around rounding for this unsigned path (e.g., values just below and above scale/2) to prevent regressions.
return U((value + scale / 2) / scale);
src/mips/psyqo/fixed-point.hh:178
- The doc comment for
integer()should explicitly mention that both signed and unsigned overloads now use the same rounding-to-nearest behavior.
/**
| } | ||
| } | ||
| return U(value + scale / 2) / U(scale); | ||
| return U((value + scale / 2) / scale); |
There was a problem hiding this comment.
The rounding logic is duplicated between signed and unsigned integer() overloads. Consider extracting a common helper to centralize this behavior and reduce duplication.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mips/psyqo/fixed-point.hh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: coverage
- GitHub Check: build
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: build-openbios
- GitHub Check: aur-build
- GitHub Check: toolchain
- GitHub Check: asan
- GitHub Check: cross-arm64
| } | ||
| } | ||
| return U(value + scale / 2) / U(scale); | ||
| return U((value + scale / 2) / scale); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LGTM! This change correctly fixes the casting precision issue.
The modification ensures that integer division occurs before type conversion, which prevents potential precision loss and inconsistent behavior when converting between signed and unsigned types. By performing (value + scale / 2) / scale in the native type T first, then casting the result to U, the function avoids intermediate casting issues that could occur when T and U have different signedness.
However, this change creates an inconsistency with other similar methods in the class. Consider applying the same pattern to the floor() and ceil() methods for consistency:
Current floor() method (lines 193, 196):
return U(value - scale + 1) / U(scale); // and
return U(value) / U(scale);Current ceil() method (lines 214, 217):
return U(value) / U(scale); // and
return U(value + scale - 1) / U(scale);Suggested refactor for consistency:
# In floor() method:
-return U(value - scale + 1) / U(scale);
+return U((value - scale + 1) / scale);
-return U(value) / U(scale);
+return U(value / scale);
# In ceil() method:
-return U(value) / U(scale);
+return U(value / scale);
-return U(value + scale - 1) / U(scale);
+return U((value + scale - 1) / scale);This would ensure all conversion methods use the same "divide first, then cast" approach for better consistency and to prevent similar casting issues.
🤖 Prompt for AI Agents
In src/mips/psyqo/fixed-point.hh around lines 193 to 196 and 214 to 217,
refactor the floor() and ceil() methods to perform the division in the native
type T before casting to U, similar to the fix applied at line 175. This means
adjusting the expressions so that all arithmetic operations happen first in type
T, then the final result is cast to U, ensuring consistent handling of
signedness and precision across all conversion methods.
The
integer()overload with a different type had inconsistent codepath between signed and unsigned, resulting in precision problems.