Skip to content

Fixing casting issue in fixed-point.hh #1941

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nicolasnoble merged 1 commit into from
Jun 8, 2025
Merged

Fixing casting issue in fixed-point.hh #1941

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/mips/psyqo/fixed-point.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ class FixedPoint {
return U((value - scale / 2) / scale);
}
}
return U(value + scale / 2) / U(scale);
return U((value + scale / 2) / scale);
Copy link

Copilot AI Jun 8, 2025

Choose a reason for hiding this comment

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

The rounding logic is duplicated between signed and unsigned integer() overloads. Consider extracting a common helper to centralize this behavior and reduce duplication.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jun 8, 2025

Choose a reason for hiding this comment

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

🛠️ 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.

}

/**
Expand Down
Loading