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.

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

core: set PROVIDER type as Persistent class id #1730

Closed

Conversation

trevnorris
Copy link
Contributor

Pass along the PROVIDER type, that is already passed to AsyncWrap, along
to BaseObject to set the handle_'s class id. This will allow all
Persistents to be transversed and uniquely identified by what type they
are using APIs such as v8::PersistentHandleVisitor.

R=@bnoordhuis

Pass along the PROVIDER type, that is already passed to AsyncWrap, along
to BaseObject to set the handle_'s class id. This will allow all
Persistents to be transversed and uniquely identified by what type they
are using APIs such as v8::PersistentHandleVisitor.
@trevnorris
Copy link
Contributor Author

For anyone wondering about possible performance implications, I haven't been able to measure any difference at even the nanosecond level of any micro-benchmarks.

@brendanashworth brendanashworth added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 19, 2015
inline BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> handle)
inline BaseObject::BaseObject(Environment* env,
v8::Local<v8::Object> handle,
const uint16_t p_id)
Copy link
Member

Choose a reason for hiding this comment

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

Can I suggest you call it class_id? p_id leaves the reader guessing.

@bnoordhuis
Copy link
Member

LGTM with a suggestion.

trevnorris added a commit that referenced this pull request May 19, 2015
Pass along the PROVIDER type, that is already passed to AsyncWrap, along
to BaseObject to set the handle_'s class id. This will allow all
Persistents to be transversed and uniquely identified by what type they
are using APIs such as v8::PersistentHandleVisitor.

PR-URL: #1730
Reviewed-By: Ben Noordhuis <[email protected]>
@trevnorris
Copy link
Contributor Author

Thanks. Landed w/ suggestion in 3c44100.

@trevnorris trevnorris closed this May 19, 2015
@trevnorris trevnorris deleted the propagate-provider-to-class-id branch May 19, 2015 15:46
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 19, 2015
Pass along the PROVIDER type, that is already passed to AsyncWrap, along
to BaseObject to set the handle_'s class id. This will allow all
Persistents to be transversed and uniquely identified by what type they
are using APIs such as v8::PersistentHandleVisitor.

PR-URL: nodejs#1730
Reviewed-By: Ben Noordhuis <[email protected]>
@rvagg rvagg mentioned this pull request May 23, 2015
andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
Pass along the PROVIDER type, that is already passed to AsyncWrap, along
to BaseObject to set the handle_'s class id. This will allow all
Persistents to be transversed and uniquely identified by what type they
are using APIs such as v8::PersistentHandleVisitor.

PR-URL: nodejs/node#1730
Reviewed-By: Ben Noordhuis <[email protected]>
@refack refack mentioned this pull request Oct 18, 2018
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants