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

Add convenience getter for Local from Persistent #492

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kkoopa
Copy link
Collaborator

@kkoopa kkoopa commented Oct 11, 2015

This is found in newer versions of V8. Needs checking that no leaks were introduced.

@rvagg
Copy link
Member

rvagg commented Oct 12, 2015

Pretty big set of changes, I won't have time to review and am probably not the best for this one anyway, maybe @bnoordhuis or @agnat if they have time.

Missing docs though, is it just Get() that we want to add or should more of this be officially documented, (like .persistent?)

@kkoopa
Copy link
Collaborator Author

kkoopa commented Oct 12, 2015

Just Get, the rest is private, so no point in documenting anything else, since it is not accessible. There were many changes, since I redid the newer implementation not to inherit from v8::PeristentBase<> but to compose it.

On October 12, 2015 2:05:26 PM GMT+03:00, Rod Vagg notifications@github.com wrote:

Pretty big set of changes, I won't have time to review and am probably
not the best for this one anyway, maybe @bnoordhuis or @agnat if they
have time.

Missing docs though, is it just Get() that we want to add or should
more of this be officially documented, (like .persistent?)


Reply to this email directly or view it on GitHub:
#492 (comment)

@brodybits
Copy link

I wonder if we can simply add Get to Nan::Persistent and Nan::Global instead? This would preserve Nan::PersistentBase and its subclasses as subclasses of v8::PersistentBase FWIW.

@kkoopa
Copy link
Collaborator Author

kkoopa commented Oct 12, 2015

Yes, that would work, but I wanted to try and redo the implementation to make
it more similar to the other one. Now the hierarchy is the same across
versions, as would be expected.

On Monday 12 October 2015 04:36:52 Chris Brody wrote:

I wonder if we can simply add Get to Nan::Persistent and Nan::Global
instead? This would preserve Nan::PersistentBase and its subclasses as
subclasses of v8::PersistentBase FWIW.


Reply to this email directly or view it on GitHub:
#492 (comment)

@brodybits
Copy link

Sounds reasonable to me. There is an assumption that the user would never need access to the v8::PersistentBase object, hope this will remain true.

@kkoopa
Copy link
Collaborator Author

kkoopa commented Oct 12, 2015

That assumption is already there for versions less than 0.12, this makes it
explicit in newer versions too. v8::PersistentBase cannot even be directly
instantiated.

On Monday 12 October 2015 04:50:21 Chris Brody wrote:

Sounds reasonable to me. There is an assumption that the user would never
need access to the v8::PersistentBase object, hope this will remain true.


Reply to this email directly or view it on GitHub:
#492 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants