Double-check mfbt/FloatingPoint.h's bitwise floating point operations for correctness

RESOLVED FIXED

Status

()

Core
MFBT
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Waldo, Assigned: jimb)

Tracking

Trunk
All
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Bug 714260 moved all the low-level floating point bit-level algorithms into mfbt/FloatingPoint.h.  At that time I thought I'd gotten enough reviews of the patch to land it.  A couple few weeks later, tho, and paranoia's starting to set in.  I'd like someone to do a last comparison of the algorithms defined there to what IEEE-754 says should be the case, specifically checking for consistency.

I *do not* believe or expect there's a difference between what's implemented and what should be implemented.  But given the JS engine's reliance on these low-level details for value representation, it's worth another look.

Given that I don't think there's a problem here, I don't think it's critical this happen before the upcoming merge.  But it should definitely happen comfortably before the merge after that, so that *if* something gets found, we can fix it in the then-aurora branch without having to mess with scheduling.  jimb says he can do this after the 30th, early in the next cycle -- unless someone complains loudly, I think that's adequate timing given I doubt anything's actually wrong.
Created attachment 615820 [details] [diff] [review]
Dummy patch on which to flag jimb -- see mfbt/FloatingPoint.h for the code to review
Attachment #615820 - Flags: review?(jimb)

Updated

5 years ago
tracking-firefox14: ? → +
Assigning to Jeff for now since he's the only one who's touching it but if another viable bug owner comes alone, who's name isn't 'nobody@mozilla.com', please reassign as needed.
Assignee: nobody → jwalden+bmo
Sorry, moving this over to jimb who's review is all that is needed here.
Assignee: jwalden+bmo → jimb
[Triage Comment]

Merging of 14 to the beta channel is happening tomorrow. What's the current game plan for this issue?  I'll remove tracking unless someone makes a case that this will be worked on in time to land in the first couple of beta builds.
Attachment #615820 - Flags: review?(jimb) → review?(nfroyd)
Apparently Splinter is confused if there's no diff.

As far as consistency goes, everything looks like it does what it says on the tin.
Attachment #615820 - Flags: review?(nfroyd) → review+
Good, good, that's what I expected.  We're all set here then (including on all branches, since there's nothing to change).
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
tracking-firefox14: + → ---
You need to log in before you can comment on or make changes to this bug.