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

quickjspp v2 #45

Draft
wants to merge 52 commits into
base: master
Choose a base branch
from
Draft

quickjspp v2 #45

wants to merge 52 commits into from

Conversation

ftk
Copy link
Owner

@ftk ftk commented Dec 11, 2021

This changes the way pointers are stored in JS objects.

v1 (branch master) v2 (branch sharedptr)
js object holds opaque pointer to std::shared_ptr<T> T
std::shared_ptr<T> conversion yes no
qjs::shared_ptr<T> conversion no yes
T* conversion yes only if T has qjs::enable_shared_from_this<T> as base class
extra allocations when converting from C++ pointer to JS object yes(slower) no(faster)
weak_ptr and deleter support std not yet
inheritance support yes limited
creating c++ objects new,std::make_shared qjs::make_shared

current v2 limitations:

  • qjs::make_shared needs JSContext
  • all qjs::shared_ptr instances need to be destroyed before js context is destroyed

Also fixes bugs where JS properties are not saved

@cykoder
Copy link

cykoder commented Dec 11, 2021

looks great, will give it a try when ready :)

@cykoder
Copy link

cykoder commented Sep 16, 2022

small bit of feedback, i tried implementing this branch over last weekend to see how itd go with the changes. requiring classes to be inherited from enable_shared_from_this is a bit of a pain in an existing codebase, was much better in V1 where things can be auto exposed. even if i want to have a class that will only be created natively in JS i still need to inherit from that which doesnt seem nessecary

@luodaoyi
Copy link

@samhellawell I'm agree with you !

enable_shared_from_this is a bad idea.

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.

3 participants