GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Replace napi_is_construct_call with napi_get_new_target by sampsongao · Pull Request #113 · nodejs/node-addon-api

Conversation

@sampsongao

@sampsongao sampsongao commented Aug 21, 2017

Copy link
Copy Markdown

For nodejs/node#14698

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Sampson Gao
Replace napi_is_construct_call with napi_get_new_target
f98a747
addaleax
addaleax approved these changes Aug 21, 2017
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

It might be pretty cheap to just introduce a CallbackInfo::NewTarget() method and make IsConstructCall() a small wrapper around it while you’re at it

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sampsongao
sampsongao requested a review from jasongin August 21, 2017 20:06
Sampson Gao
Add NewTarget method to CallbackInfo and change variable name to came…
f894a43 
…l case
addaleax
addaleax approved these changes Aug 22, 2017
View reviewed changes
Comment thread napi-inl.h Outdated
napi_value newTarget;
napi_status status = napi_get_new_target(_env, _info, &newTarget);
bool isConstructCall = (new_target != nullptr);
NAPI_THROW_IF_FAILED(_env, status, false);

@addaleax addaleax Aug 22, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Shouldn’t this line be part of NewTarget() as well?

By the way, what I had in mind was just replacing IsConstructCall with

inline bool CallbackInfo::IsConstructCall() const {
  return !NewTarget().IsEmpty();
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Sampson Gao
Add missing error handling check in CallbackInfo::NewTarget()
0eda0d6
@sampsongao
sampsongao force-pushed the remove_is_construct_call branch from 2dc38a9 to 0eda0d6 Compare August 22, 2017 21:16
mhdawson
mhdawson approved these changes Aug 22, 2017
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM but we need to make sure not to land until the corresponding change goes out in an N-API release.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Sampson Gao added 2 commits August 22, 2017 18:59
Sampson Gao
Fix typo
739d44e
Sampson Gao
Reduce duplicated code
17cd24f
@sampsongao

sampsongao commented Sep 15, 2017

Copy link
Copy Markdown
Author

Landed 458f576
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sampsongao sampsongao closed this Sep 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes
@mhdawson mhdawson mhdawson approved these changes
@jasongin jasongin Awaiting requested review from jasongin

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@sampsongao @addaleax @mhdawson
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.