Last Comment Bug 746273 - Double-check mfbt/FloatingPoint.h's bitwise floating point operations for correctness
: Double-check mfbt/FloatingPoint.h's bitwise floating point operations for cor...
Product: Core
Classification: Components
Component: MFBT (show other bugs)
: Trunk
: All Windows 2000
: -- normal (vote)
: ---
Assigned To: Jim Blandy :jimb
Depends on:
Blocks: 714260
  Show dependency treegraph
Reported: 2012-04-17 12:16 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-06-14 08:48 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Dummy patch on which to flag jimb -- see mfbt/FloatingPoint.h for the code to review (1 bytes, patch)
2012-04-17 12:26 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
nfroyd: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-17 12:16:16 PDT
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.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-17 12:26:07 PDT
Created attachment 615820 [details] [diff] [review]
Dummy patch on which to flag jimb -- see mfbt/FloatingPoint.h for the code to review
Comment 2 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-02 15:46:54 PDT
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 '', please reassign as needed.
Comment 3 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-02 15:49:45 PDT
Sorry, moving this over to jimb who's review is all that is needed here.
Comment 4 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-03 10:58:01 PDT
[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.
Comment 5 Nathan Froyd [:froydnj] 2012-06-08 12:45:08 PDT
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.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-08 13:47:41 PDT
Good, good, that's what I expected.  We're all set here then (including on all branches, since there's nothing to change).

Note You need to log in before you can comment on or make changes to this bug.