Bug 143865 - Implement `Object.is`
Summary: Implement `Object.is`
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jordan Harband
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-17 00:54 PDT by Jordan Harband
Modified: 2015-04-20 00:06 PDT (History)
4 users (show)

See Also:


Attachments
Patch (9.51 KB, patch)
2015-04-19 11:36 PDT, Jordan Harband
no flags Details | Formatted Diff | Diff
Patch (9.54 KB, patch)
2015-04-19 11:45 PDT, Jordan Harband
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-mavericks (542.31 KB, application/zip)
2015-04-19 12:19 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (595.77 KB, application/zip)
2015-04-19 12:24 PDT, Build Bot
no flags Details
Patch (9.57 KB, patch)
2015-04-19 13:33 PDT, Jordan Harband
no flags Details | Formatted Diff | Diff
Patch (9.70 KB, patch)
2015-04-19 15:21 PDT, Jordan Harband
no flags Details | Formatted Diff | Diff
Patch (9.64 KB, patch)
2015-04-19 17:36 PDT, Jordan Harband
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jordan Harband 2015-04-17 00:54:43 PDT
This function simply exposes `sameValue` to JS.
Comment 1 Jordan Harband 2015-04-19 11:36:41 PDT
Created attachment 251121 [details]
Patch
Comment 2 Jordan Harband 2015-04-19 11:45:05 PDT
Created attachment 251122 [details]
Patch
Comment 3 shalecraig 2015-04-19 11:49:04 PDT
For PropertyDescriptor.cpp[0], why not just this:

if (std::isnan(x) && std::isnan(y))
    return true;

[0] https://bugs.webkit.org/attachment.cgi?id=251122&action=diff#a/Source/JavaScriptCore/runtime/PropertyDescriptor.cpp_sec1
Comment 4 Build Bot 2015-04-19 12:19:10 PDT
Comment on attachment 251122 [details]
Patch

Attachment 251122 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4778729989472256

New failing tests:
js/Object-is.html
Comment 5 Build Bot 2015-04-19 12:19:14 PDT
Created attachment 251124 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 6 Build Bot 2015-04-19 12:24:02 PDT
Comment on attachment 251122 [details]
Patch

Attachment 251122 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5054959737372672

New failing tests:
js/Object-is.html
Comment 7 Build Bot 2015-04-19 12:24:04 PDT
Created attachment 251125 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 8 Jordan Harband 2015-04-19 13:33:38 PDT
Created attachment 251127 [details]
Patch
Comment 9 Jordan Harband 2015-04-19 13:33:53 PDT
Comment on attachment 251124 [details]
Archive of layout-test-results from ews103 for mac-mavericks

my bsd
Comment 10 Jordan Harband 2015-04-19 13:37:37 PDT
(In reply to comment #3)
> For PropertyDescriptor.cpp[0], why not just this:
> 
> if (std::isnan(x) && std::isnan(y))
>     return true;
> 
> [0]
> https://bugs.webkit.org/attachment.cgi?id=251122&action=diff#a/Source/
> JavaScriptCore/runtime/PropertyDescriptor.cpp_sec1

If one is NaN and the other's not, there's no purpose in casting to a number, it'd be false. The logic is more like `if (xNaN || yNaN) { return xNaN && yNaN }`
Comment 11 Jordan Harband 2015-04-19 15:21:20 PDT
Created attachment 251131 [details]
Patch
Comment 12 Darin Adler 2015-04-19 16:52:39 PDT
Comment on attachment 251131 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251131&action=review

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:588
> +    JSValue value1 = exec->argument(0);
> +    JSValue value2 = exec->argument(1);
> +    return JSValue::encode(jsBoolean(sameValue(exec, value1, value2)));

No reason to make these local variables, since there is no side effect here. This would read better as a one liner:

    return JSValue::encode(jsBoolean(sameValue(exec, exec->argument(0), exec->argument(1))));
Comment 13 Jordan Harband 2015-04-19 17:35:13 PDT
(In reply to comment #12)
> Comment on attachment 251131 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251131&action=review
> 
> > Source/JavaScriptCore/runtime/ObjectConstructor.cpp:588
> > +    JSValue value1 = exec->argument(0);
> > +    JSValue value2 = exec->argument(1);
> > +    return JSValue::encode(jsBoolean(sameValue(exec, value1, value2)));
> 
> No reason to make these local variables, since there is no side effect here.
> This would read better as a one liner:
> 
>     return JSValue::encode(jsBoolean(sameValue(exec, exec->argument(0),
> exec->argument(1))));


I tend to prefer intermediate variables for readability and step-through debugging, but sounds good, I'll make this change right now
Comment 14 Jordan Harband 2015-04-19 17:36:09 PDT
Created attachment 251140 [details]
Patch
Comment 15 WebKit Commit Bot 2015-04-20 00:06:48 PDT
Comment on attachment 251140 [details]
Patch

Clearing flags on attachment: 251140

Committed r183006: <http://trac.webkit.org/changeset/183006>
Comment 16 WebKit Commit Bot 2015-04-20 00:06:52 PDT
All reviewed patches have been landed.  Closing bug.