Summary: | Some Math.pow test262 tests fail with MinGW-w64 | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jonathan Liu <net147> | ||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | barraclough, hausmann, kai.koehne, mhahnenberg, sam, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 103747 | ||||||||||||
Attachments: |
|
Description
Jonathan Liu
2012-12-15 05:44:35 PST
Created attachment 179590 [details]
Patch
Attachment 179590 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/runtime/MathObject.cpp:236: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5]
Source/JavaScriptCore/runtime/MathObject.cpp:245: y_int is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
Source/JavaScriptCore/runtime/MathObject.cpp:246: More than one command on the same line in if [whitespace/parens] [4]
Total errors found: 3 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 179592 [details]
Patch
Attachment 179592 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/runtime/MathObject.cpp:236: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5]
Source/JavaScriptCore/runtime/MathObject.cpp:246: More than one command on the same line in if [whitespace/parens] [4]
Total errors found: 2 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 179593 [details]
Patch
Comment on attachment 179593 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179593&action=review > Source/JavaScriptCore/runtime/MathObject.cpp:235 > +#ifdef __MINGW64_VERSION_MAJOR I think this should be #if COMPILER(MINGW64) > Source/JavaScriptCore/runtime/MathObject.cpp:244 > + if ((x == 0.0 || isinf(x)) && isfinite(y)) { > + double f; > + if (modf(y, &f) != 0.0) > + return ((x == 0.0) ^ (y > 0.0)) ? std::numeric_limits<double>::infinity() : 0.0; > + } > + > + if (x == 2.0) { Isn't it dangerous to compare doubles to constants like that? Shouldn't it be done as a range comparison using a epsilon? Created attachment 179707 [details]
Patch
The special cases apply only to exact values so it is fine in my opinion. Upstream "discussion" about the behavioral difference: http://comments.gmane.org/gmane.comp.gnu.mingw.w64.general/4658 Comment on attachment 179707 [details] Patch Clearing flags on attachment: 179707 Committed r137895: <http://trac.webkit.org/changeset/137895> All reviewed patches have been landed. Closing bug. I don't think this is the best way to do this. The preferred method is to add a function to MathExtras.h which implements pow correctly and call that, so that we don't litter the code with #ifdefs. (In reply to comment #12) > I don't think this is the best way to do this. The preferred method is to add a function to MathExtras.h which implements pow correctly and call that, so that we don't litter the code with #ifdefs. According to the bug life cycle, this bug is fixed. Feel free to commit such a change separately. I've filed bug #105925 to address Sam's (valid) comment. |