Object.fromEntries is a stage 3 proposal: https://github.com/tc39/proposal-object-from-entries It'd be great to get it implemented in Webkit/Safari soon! Test262 tests are here: https://github.com/tc39/test262/pull/1676 Ref: https://github.com/tc39/proposal-object-from-entries/issues/21
Created attachment 346964 [details] Patch
Created attachment 346965 [details] Patch
Created attachment 346967 [details] Patch
Comment on attachment 346967 [details] Patch Thanks, this looks great!
Here are the kinds of things I think about when adding tests for an operation like this one: Imagine that someone is trying to optimize the implementation of this and write the function some different way, not in JavaScript. I want tests to cover what that person might get wrong, such as: 1) Verify no additional properties, including but not limited to "length", are "get" and no properties "set" on the iterable passed in and on the objects it returns. 2) Verify that prototypes are consulted properly when properties are missing on the objects returned by the iterable. Perhaps we'd want to test a prototype chain of more than one? 3) Verify that the operations are done in the order mentioned in the specification, for example, get "0" before "1"; this typically requires passing objects that produce side effects from the "get" operation. But even for tests that involve prototypes, test that operations are done in the correct order. 3a) Verify that exceptions raised are propagated correctly. 4) Verify that the behavior is correct when the object has no "0" or "1" property; related to the prototype tests, but the key is that lack of property turns into "undefined", if that’s the correct behavior. 5) Verify that there is no accidental transformation of either keys or values: null doesn't become undefined or vice versa, no conversion to string, etc. 6) Check what goes on in exotic cases like where the Object prototype has a non-standard value. I’d like to see at least some of these tests be a standard practice when we add JavaScript functions. A major goal of these tests is to protect us from behavior changes if we later decide to do a different implementation. Also very useful for checking across other JavaScript implementations.
There's existing test262 tests; can that cover it, or does webkit need its own separate tests?
(In reply to Jordan Harband from comment #6) > There's existing test262 tests Great. > can that cover it, or does webkit need its own separate tests? Sure, those are excellent, as long as we check them in to the WebKit tree and run them. This patch doesn’t show me new successes on any tests so I assume that means they are not in the tree yet. If the test262 tests don’t cover all the things I suggested above, then I suggest we add coverage to them.
(In reply to Darin Adler from comment #5) > Here are the kinds of things I think about when adding tests for an > operation like this one: > > Imagine that someone is trying to optimize the implementation of this and > write the function some different way, not in JavaScript. I want tests to > cover what that person might get wrong, such as: > > 1) Verify no additional properties, including but not limited to "length", > are "get" and no properties "set" on the iterable passed in and on the > objects it returns. > > 2) Verify that prototypes are consulted properly when properties are missing > on the objects returned by the iterable. Perhaps we'd want to test a > prototype chain of more than one? > > 3) Verify that the operations are done in the order mentioned in the > specification, for example, get "0" before "1"; this typically requires > passing objects that produce side effects from the "get" operation. But even > for tests that involve prototypes, test that operations are done in the > correct order. > > 3a) Verify that exceptions raised are propagated correctly. > > 4) Verify that the behavior is correct when the object has no "0" or "1" > property; related to the prototype tests, but the key is that lack of > property turns into "undefined", if that’s the correct behavior. > > 5) Verify that there is no accidental transformation of either keys or > values: null doesn't become undefined or vice versa, no conversion to > string, etc. > > 6) Check what goes on in exotic cases like where the Object prototype has a > non-standard value. > > I’d like to see at least some of these tests be a standard practice when we > add JavaScript functions. A major goal of these tests is to protect us from > behavior changes if we later decide to do a different implementation. Also > very useful for checking across other JavaScript implementations. Cool, I'll update the patch. (In reply to Jordan Harband from comment #4) > Comment on attachment 346967 [details] > Patch > > Thanks, this looks great! Thanks for your informal review :) And please do not set r+ for the patch in informal review since r+ from reviewers is only effective.
whoops, sorry :-) i assumed it worked like github, where it only counts +r's from people with the collaborator bit.
(In reply to Jordan Harband from comment #9) > whoops, sorry :-) i assumed it worked like github, where it only counts +r's > from people with the collaborator bit. No problem!! :D
Oh, I guess I didn't need to set review+ because Yusuke will upload a new version for review soon.
(In reply to Darin Adler from comment #11) > Oh, I guess I didn't need to set review+ because Yusuke will upload a new > version for review soon. Oops, sorry. I was working on promise rejection in workers and some JSC issues found by UBSan :) I'll land it with suggested tests!
Did not mean to rush you at all. Just didn’t want to leave you hanging.
Comment on attachment 346967 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346967&action=review LGTM too, just a couple of suggestions. > Source/JavaScriptCore/builtins/ObjectConstructor.js:48 > + var object = {}; Style nit: Maybe use “let” in this function? > Source/JavaScriptCore/builtins/ObjectConstructor.js:50 > + for (var entry of iterable) { Can we add tests for all the effects this function does? - Iterator protocol - Get on key 0 and 1
Comment on attachment 346967 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346967&action=review Thanks, I've added suggested tests. >> Source/JavaScriptCore/builtins/ObjectConstructor.js:48 >> + var object = {}; > > Style nit: Maybe use “let” in this function? Changed. >> Source/JavaScriptCore/builtins/ObjectConstructor.js:50 >> + for (var entry of iterable) { > > Can we add tests for all the effects this function does? > - Iterator protocol > - Get on key 0 and 1 Added both!
Committed r235589: <https://trac.webkit.org/changeset/235589>
<rdar://problem/44052565>