Skip to content

Expose JsHasOwnProperty#3316

Merged
chakrabot merged 1 commit into
chakra-core:release/1.7from
kfarnung:hasownprop
Jul 11, 2017
Merged

Expose JsHasOwnProperty#3316
chakrabot merged 1 commit into
chakra-core:release/1.7from
kfarnung:hasownprop

Conversation

@kfarnung
Copy link
Copy Markdown
Contributor

@kfarnung kfarnung commented Jul 11, 2017

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

@kfarnung
Copy link
Copy Markdown
Contributor Author

kfarnung commented Jul 11, 2017

@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 kfarnung requested review from digitalinfinity, kunalspathak and mrkmarron and removed request for digitalinfinity and kunalspathak July 11, 2017 01:37
@kfarnung
Copy link
Copy Markdown
Contributor Author

kfarnung commented Jul 11, 2017

/cc @liminzhu

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

@kunalspathak kunalspathak Jul 11, 2017

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
Copy Markdown
Contributor Author

@kfarnung kfarnung Jul 11, 2017

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
Copy Markdown
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
Copy Markdown
Collaborator

obastemur commented Jul 11, 2017

LGTM

kfarnung
kfarnung commented Jul 11, 2017
View reviewed changes
Comment thread lib/Runtime/Debug/TTEvents.h Outdated

Count
Count,
HasOwnPropertyActionTag
Copy link
Copy Markdown
Contributor Author

@kfarnung kfarnung Jul 11, 2017

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
Copy Markdown
Contributor Author

kfarnung commented Jul 11, 2017

@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
Copy Markdown
Contributor

mrkmarron commented Jul 11, 2017 via email

@liminzhu
Copy link
Copy Markdown
Collaborator

liminzhu commented Jul 11, 2017

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
Copy Markdown
Contributor Author

kfarnung commented Jul 11, 2017

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

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

/cc @kunalspathak @liminzhu

@liminzhu
Copy link
Copy Markdown
Collaborator

liminzhu commented Jul 12, 2017

@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

@mrkmarron mrkmarron Awaiting requested review from mrkmarron

@digitalinfinity digitalinfinity Awaiting requested review from digitalinfinity

1 more reviewer

@kunalspathak kunalspathak kunalspathak approved these changes

Reviewers whose approvals may not affect merge requirements

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