Closed
Bug 777494
Opened 13 years ago
Closed 13 years ago
Avoid using == with float
Categories
(Core :: Layout: Text and Fonts, defect)
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: espindola, Unassigned)
Details
Attachments
(1 file)
2.92 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
layout/generic/nsTextFrameThebes.cpp currently has some equality comparison with float point. While it looks like they should be valid in a really strict compiler, on x86 there can be some problems.
The x87 stack has higher precision than a float. If one side of the comparison is coming from a computation on the stack and the other one is loaded from memory, there can be a small difference that causes the comparison to fail.
Attachment #645875 -
Flags: review?(roc)
Reporter | ||
Comment 1•13 years ago
|
||
A try push is at https://tbpl.mozilla.org/?tree=Try&pusher=respindola@mozilla.com.
Attachment #645875 -
Flags: review?(roc) → review+
Comment 2•13 years ago
|
||
Comment on attachment 645875 [details] [diff] [review]
don't use == with float
Review of attachment 645875 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/generic/nsTextFrameThebes.cpp
@@ +6710,5 @@
> }
> }
>
> +bool nsTextFrame::IsCurrentFontInflation(float aInflation) const {
> + return fabsf(aInflation - GetFontSizeInflation()) < 1e-6;
If aInflation is greater than 1e6 * epsilon, which is roughly 1e+1, then this is no different than the == comparison it replaces.
So:
- either you know something very specific here about the order of magnitude of aInflation?
- or you should use a relative (multiplicative), not absolute, fuzzy comparison. Typically:
abs(a - b) < precision * min(abs(a), abs(b))
Reporter | ||
Comment 3•13 years ago
|
||
From what I see in the code, the comparison is always used to check if a value is from a previous call to GetFontSizeInflation. The case I noticed it failing was because one side had been stored (narrowed to a float) and the other wasn't. The difference from the long double to the float was about 1e-8, so I just used 1e-6, but I agree it is not a general solution.
Do we have a routine for relative comparison somewhere in the codebase?
Font inflation values will generally be between 0.1 and 10 so this is fine.
Comment 5•13 years ago
|
||
btw, gcc >= 3.0 has a -Wfloat-equal warning flag that will warn when a floating point value is compared for equality using the == or != operators.
http://gcc.gnu.org/onlinedocs/gcc-3.0.4/gcc_3.html#SEC11
Comment 6•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment 7•13 years ago
|
||
(In reply to Chris Peterson (:cpeterson) from comment #5)
> btw, gcc >= 3.0 has a -Wfloat-equal warning flag that will warn when a
> floating point value is compared for equality using the == or != operators.
>
> http://gcc.gnu.org/onlinedocs/gcc-3.0.4/gcc_3.html#SEC11
Filed bug 777803 to enable this warning.
You need to log in
before you can comment on or make changes to this bug.
Description
•