fix Double formatting missing sign when rounding to zero #3
Conversation
Summary of ChangesHello @tayloraswift, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a formatting bug in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where the sign was missing when formatting a Double that rounds to zero. The change introduces a special case to handle this and adds unit tests to verify the behavior. The fix is mostly correct, but it fails to handle -0.0 because of how it determines the sign of the original number. I've suggested a correction for this. I've also suggested expanding the new test suite to cover +0.0 and -0.0 explicitly, which would have caught this issue.
| if signed, decimal.units == 0, | ||
| let sign: Bool = self.sign { | ||
| // special case to ensure sign is shown for zero when requested | ||
| return places > 0 ? """ | ||
| \(sign ? "+" : "\u{2212}")0.\(String.init(repeating: "0", count: places))\(suffix) | ||
| """ : """ | ||
| \(sign ? "+" : "\u{2212}")0\(suffix) | ||
| """ | ||
| } else { | ||
| return decimal.format(power: power, places: places, signed: signed, suffix: suffix) | ||
| } |
There was a problem hiding this comment.
This special case logic for signed zeros is a great addition, but it has a flaw: it doesn't correctly handle -0.0. The custom self.sign property returns nil for both 0.0 and -0.0 (because self == 0 is true for both), so the optional binding let sign: Bool = self.sign fails and this block is not entered for -0.0.
To correctly handle all cases, including -0.0, you should use self.signbit. self.signbit is true for negative numbers (including -0.0) and false for positive ones.
My suggestion below fixes this and also simplifies the multiline string literals into single-line strings for conciseness.
if signed, decimal.units == 0 {
// `self.sign` is nil for `0.0` and `-0.0`, so `self.signbit` is used instead to
// correctly determine the sign for a signed zero.
let signIsPositive: Bool = !self.signbit
// special case to ensure sign is shown for zero when requested
return places > 0 ?
"\(signIsPositive ? "+" : "\u{2212}")0.\(String(repeating: "0", count: places))\(suffix)" :
"\(signIsPositive ? "+" : "\u{2212}")0\(suffix)"
} else {
return decimal.format(power: power, places: places, signed: signed, suffix: suffix)
}| import D | ||
| import Testing | ||
|
|
||
| @Suite struct FloatingPointFormattingTests { |
There was a problem hiding this comment.
These are great tests that cover the core of the bug fix! To make the test suite even more robust, consider adding tests specifically for 0.0 and -0.0 to ensure they are formatted correctly with signs when requested. This would have helped catch the subtle issue with using self.sign in the implementation.
For example:
@Test func SignedZero() {
let a: Double = 0.0
#expect("\(+a[..1])" == "+0.0")
let b: Double = -0.0
#expect("\(+b[..1])" == "−0.0")
}
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where the sign was missing when formatting a Double that rounds to zero. The change correctly identifies this case and prepends the appropriate sign. The logic is sound and is accompanied by a good set of new unit tests that verify the fix and related rounding behaviors. I have one suggestion to improve the readability of the new formatting logic.
| return places > 0 ? """ | ||
| \(sign ? "+" : "\u{2212}")0.\(String.init(repeating: "0", count: places))\(suffix) | ||
| """ : """ | ||
| \(sign ? "+" : "\u{2212}")0\(suffix) | ||
| """ |
There was a problem hiding this comment.
This block with the ternary operator and multiline string literals can be simplified to a more concise and readable single-line expression. This avoids the verbosity of """ for what is essentially single-line string construction.
return (sign ? "+" : "\u{2212}") + (places > 0 ? "0.\(String(repeating: "0", count: places))" : "0") + suffix
No description provided.