Bug 9678 - fmod function of MS CRT returns NaN for x / Inf
Summary: fmod function of MS CRT returns NaN for x / Inf
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-07-01 15:09 PDT by Björn Graf (boki)
Modified: 2006-07-04 21:07 PDT (History)
1 user (show)

See Also:


Attachments
MS CRT fmod bug workaround where x % (1/0) = NaN, not x. (1.57 KB, patch)
2006-07-01 15:11 PDT, Björn Graf (boki)
darin: review-
Details | Formatted Diff | Diff
Workaround for MSVCRTs fmod function (2.29 KB, patch)
2006-07-03 20:20 PDT, Björn Graf (boki)
mjs: review+
Details | Formatted Diff | Diff
my alternate version of the fmod patch (4.88 KB, patch)
2006-07-04 12:25 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
my alternate version of the fmod patch (7.48 KB, patch)
2006-07-04 16:40 PDT, Darin Adler
mjs: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Björn Graf (boki) 2006-07-01 15:09:14 PDT
JavaScriptCore expects fmod to return Inf for finite dividend and infinite divisor , but the MS CRT version of fmod returns NaN in these cases.
Comment 1 Björn Graf (boki) 2006-07-01 15:11:38 PDT
Created attachment 9128 [details]
MS CRT fmod bug workaround where x % (1/0) = NaN, not x.

The workaround checks the arguments for infinity and only calls fmod when the divisor is not infinite. Otherwise it returns the dividend.
Comment 2 Darin Adler 2006-07-03 18:05:48 PDT
Comment on attachment 9128 [details]
MS CRT fmod bug workaround where x % (1/0) = NaN, not x.

I'd like to see this workaround in MathExtras.h rather than inline in the JavaScript-specific files. We just need a name for the fmod with the workaround, and then we can use that. It can be an inline that expands to a call to fmod on platforms other than Win32.

Also, we do COMPILER(MSVC) rather than _MSC_VER for these kinds of ifdefs.

I don't think we need PLATFORM(WIN) at all -- probably COMPILER(MSVC) is the whole check needed.
Comment 3 Björn Graf (boki) 2006-07-03 20:20:49 PDT
Created attachment 9183 [details]
Workaround for MSVCRTs fmod function

This patch implements the necessary workaround in wtf/MathExtras.h.

Defining fmod inside the KJS namespace would have worked, if all call sites where in the namespace explicitly. However, being implicitly declared in the namespace and defined in it via the using namespace KJS; directive requires the #define workaround.

Needed to change isinf() because NaN is not finite.
Comment 4 Maciej Stachowiak 2006-07-03 23:54:07 PDT
I think Darin's suggestion was to just use an alternate function name besides fmod at the call sites, and for non-MSVC compilers it could just expand inline to fmod. It could just be called _fmod, or fremainder or something. I actually think this solution is fine though, so I'll just r+ it.
Comment 5 Darin Adler 2006-07-04 10:43:03 PDT
(In reply to comment #4)
> I think Darin's suggestion was to just use an alternate function name besides
> fmod at the call sites, and for non-MSVC compilers it could just expand inline
> to fmod. It could just be called _fmod, or fremainder or something. I actually
> think this solution is fine though, so I'll just r+ it.

Maciej's right about what I was suggesting. But I think it's also OK to use a trick so it can still be fmod at the call site. I would not have called the other function _fmod, though, because those identifiers are specifically reserved for the implementation! Maybe fmodWithWorkaround or something like that.

Also, if we are doing a macro, a macro that takes parameters is safer than one that just does the bare name. But I also think this is OK as-is, so I'll just leave Maciej's review flag intact. We can always enhance the technique used in MathExtras.h when and if we run into trouble.
Comment 6 Darin Adler 2006-07-04 12:25:52 PDT
Created attachment 9193 [details]
my alternate version of the fmod patch
Comment 7 Darin Adler 2006-07-04 16:40:37 PDT
Created attachment 9198 [details]
my alternate version of the fmod patch
Comment 8 Maciej Stachowiak 2006-07-04 20:45:49 PDT
Comment on attachment 9198 [details]
my alternate version of the fmod patch

This looks ok too, but I kinda like MathExtras.h being transparent and letting you use the normal function names. Saying wtf_fmod all over (w/ potential to mistakenly say fmod) seems worse to me than a macro solution. Agreed though that a macro with parameters would be better.
Comment 9 Maciej Stachowiak 2006-07-04 20:45:49 PDT
Comment on attachment 9198 [details]
my alternate version of the fmod patch

This looks ok too, but I kinda like MathExtras.h being transparent and letting you use the normal function names. Saying wtf_fmod all over (w/ potential to mistakenly say fmod) seems worse to me than a macro solution. Agreed though that a macro with parameters would be better.
Comment 10 Darin Adler 2006-07-04 21:07:51 PDT
Committed revision 15155.