Implementing Object.values and Object.entries by akroshg · Pull Request #171 · chakra-core/ChakraCore (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation8 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Hi @akroshg, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
It looks like you're a Microsoft contributor (Akrosh Gandhi). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.
TTYL, MSBOT;
LGTM from a compliance angle :-)
| Assert(scriptContext != nullptr); |
|---|
| JavascriptArray* valuesArray = scriptContext->GetLibrary()->CreateArray(0); |
| Var ownKeysVar = JavascriptOperators::GetOwnEnumerablePropertyNames(object, scriptContext); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assume we have a non-enumerable property when this call is made. Between here and the IsEnumerable call on line 1134, it looks like it's possible to make the property enumerable. Wouldn't it be excluded from the result?
I see that we have tests for the opposite direction (i.e., enumerable -> non-enumerable), but we're missing this case.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch - the spec says that the list of existing own keys is fixed at the initial call, but that means a preexisting own key's enumerability can change in either direction during enumeration.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@goyakin, I'll get back to you on this. It is good test case to add.
@goyakin Yes there was issue in making non-enumerable to enumerable while iteration. I have fixed that and added the test cases.
chakrabot pushed a commit that referenced this pull request
