Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Expose JsHasOwnProperty #3316

Merged
merged 1 commit into from
Jul 11, 2017
Merged

Expose JsHasOwnProperty #3316

merged 1 commit into from
Jul 11, 2017

Conversation

kfarnung
Copy link
Contributor

Enable support for usage in node's n-api. Added a new unit test for
the functionality.

@kfarnung
Copy link
Contributor Author

@obastemur This may need to be updated based on the outcome of #3207. I just followed the existing pattern for JsHasProperty before I saw that PR.

@kfarnung
Copy link
Contributor Author

/cc @liminzhu

kunalspathak
kunalspathak reviewed Jul 11, 2017
View reviewed changes
lib/Jsrt/Jsrt.cpp Outdated
VALIDATE_INCOMING_OBJECT(object, scriptContext);
VALIDATE_INCOMING_PROPERTYID(propertyId);
PARAM_NOT_NULL(hasOwnProperty);
*hasOwnProperty = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*hasOwnProperty = nullptr; [](start = 7, length = 27)

shouldn't be initialized to false? I know at other places these variables are initialized to nullptr, but it should be false in those cases as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, that came up in @obastemur PR as well, I'll take a quick scan and fix them up.

kunalspathak
kunalspathak approved these changes Jul 11, 2017
View reviewed changes
Copy link
Contributor

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add this to the wiki as well.

@obastemur
Copy link
Collaborator

LGTM

kfarnung
kfarnung commented Jul 11, 2017
View reviewed changes
lib/Runtime/Debug/TTEvents.h Outdated
@@ -169,7 +169,8 @@ namespace TTD
CodeParseActionTag,
CallExistingFunctionActionTag,

Count
Count,
HasOwnPropertyActionTag
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was dumb.

@kfarnung
Expose JsHasOwnProperty
4ba4c1a 
Enable support for usage in node's n-api. Added a new unit test for
the functionality.
@kfarnung
Copy link
Contributor Author

@mrkmarron Can you quickly sanity check my TTD changes? I'm going to need to write a new test to validate them.

@mrkmarron
Copy link
Contributor

mrkmarron commented Jul 11, 2017 via email

@liminzhu
Copy link
Collaborator

LGTM

@chakrabot chakrabot merged commit 4ba4c1a into chakra-core:release/1.7 Jul 11, 2017
chakrabot pushed a commit that referenced this pull request Jul 11, 2017
@kfarnung
[MERGE #3316 @kfarnung] Expose JsHasOwnProperty
6450f4f 
Merge pull request #3316 from kfarnung:hasownprop

Enable support for usage in node's n-api. Added a new unit test for the functionality.
chakrabot pushed a commit that referenced this pull request Jul 11, 2017
@kfarnung
[1.7>master] [MERGE #3316 @kfarnung] Expose JsHasOwnProperty
c339739 
Merge pull request #3316 from kfarnung:hasownprop

Enable support for usage in node's n-api. Added a new unit test for the functionality.
@kfarnung kfarnung deleted the hasownprop branch July 11, 2017 20:20
@kfarnung
Copy link
Contributor Author

Added a page to the wiki and updated the reference list:

https://github.com/Microsoft/ChakraCore/wiki/JsHasOwnProperty

/cc @kunalspathak @liminzhu

@liminzhu
Copy link
Collaborator

@kfarnung awesome! I added an experimental note to the wiki temporarily. Seems like a great addition to Chakra APIs as well and this is just in case Windows team has some feedback along the process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kunalspathak kunalspathak kunalspathak approved these changes

@mrkmarron mrkmarron Awaiting requested review from mrkmarron

@digitalinfinity digitalinfinity Awaiting requested review from digitalinfinity

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@kfarnung @obastemur @mrkmarron @liminzhu @kunalspathak @msftclas @chakrabot