diff --git a/Lib/javascript/jsc/javascriptcode.swg b/Lib/javascript/jsc/javascriptcode.swg index 0ad1e364d..40ce9c9b0 100644 --- a/Lib/javascript/jsc/javascriptcode.swg +++ b/Lib/javascript/jsc/javascriptcode.swg @@ -113,6 +113,25 @@ void $jswrapper(JSObjectRef thisObject) } %} +/* ----------------------------------------------------------------------------- + * js_dtor: template for a destructor wrapper + * - $jsmangledname: mangled class name + * - $jstype: class type + * - ${destructor_action}: The custom destructor action to invoke. + * ----------------------------------------------------------------------------- */ +%fragment ("js_dtoroverride", "templates") +%{ +void $jswrapper(JSObjectRef thisObject) +{ + SWIG_PRV_DATA* t = (SWIG_PRV_DATA*) JSObjectGetPrivate(thisObject); + if(t && t->swigCMemOwn) { + $jstype* arg1 = ($jstype*)t->swigCObject; + ${destructor_action} + } + if(t) free(t); +} +%} + /* ----------------------------------------------------------------------------- * js_getter: template for getter function wrappers * - $jswrapper: wrapper function name diff --git a/Lib/javascript/v8/javascriptcode.swg b/Lib/javascript/v8/javascriptcode.swg index f86198622..62267ae52 100644 --- a/Lib/javascript/v8/javascriptcode.swg +++ b/Lib/javascript/v8/javascriptcode.swg @@ -114,13 +114,32 @@ fail: void $jswrapper(v8::Persistent< v8::Value > object, void *parameter) { SWIGV8_Proxy* proxy = (SWIGV8_Proxy*) parameter; if(proxy->swigCMemOwn && proxy->swigCObject) { - std::cout << "Deleting wrapped instance: " << proxy->info->name << std::endl; +// std::cout << "Deleting wrapped instance: " << proxy->info->name << std::endl; $jsfree proxy->swigCObject; } delete proxy; } %} +/* ----------------------------------------------------------------------------- + * js_dtoroverride: template for a destructor wrapper + * - $jsmangledname: mangled class name + * - $jstype: class type + * - ${destructor_action}: The custom destructor action to invoke. + * ----------------------------------------------------------------------------- */ +%fragment ("js_dtoroverride", "templates") +%{ +void $jswrapper(v8::Persistent< v8::Value > object, void *parameter) { + SWIGV8_Proxy* proxy = (SWIGV8_Proxy*) parameter; + if(proxy->swigCMemOwn && proxy->swigCObject) { +// std::cout << "Deleting wrapped instance: " << proxy->info->name << std::endl; + $jstype arg1 = ($jstype)t->swigCObject; + ${destructor_action} + } + delete proxy; +} +%} + /* ----------------------------------------------------------------------------- * js_getter: template for getter function wrappers * - $jswrapper: wrapper function name diff --git a/Source/Modules/javascript.cxx b/Source/Modules/javascript.cxx index 8850ebd60..a8f1b0b0a 100644 --- a/Source/Modules/javascript.cxx +++ b/Source/Modules/javascript.cxx @@ -886,7 +886,6 @@ int JSEmitter::emitCtor(Node *n) { int JSEmitter::emitDtor(Node *n) { - Template t_dtor = getTemplate("js_dtor"); String *wrap_name = Swig_name_wrapper(Getattr(n, "sym:name")); SwigType *type = state.clazz(TYPE); @@ -894,20 +893,116 @@ int JSEmitter::emitDtor(Node *n) { String *ctype = SwigType_lstr(p_classtype, ""); String *free = NewString(""); + // (Taken from JSCore implementation.) + /* The if (Extend) block was taken from the Ruby implementation. + * The problem is that in the case of an %extend to create a destructor for a struct to coordinate automatic memory cleanup with the Javascript collector, + * the swig function was not being generated. More specifically: + struct MyData { + %extend { + ~MyData() { + FreeData($self); + } + } + }; + %newobject CreateData; + struct MyData* CreateData(void); + %delobject FreeData; + void FreeData(struct MyData* the_data); + + where the use case is something like: + var my_data = example.CreateData(); + my_data = null; + + This function was not being generated: + SWIGINTERN void delete_MyData(struct MyData *self){ + FreeData(self); + } + + I don't understand fully why it wasn't being generated. It just seems to happen in the Lua generator. + There is a comment about staticmemberfunctionHandler having an inconsistency and I tracked down dome of the SWIGINTERN void delete_* + code to that function in the Language base class. + The Ruby implementation seems to have an explicit check for if(Extend) and explicitly generates the code, so that's what I'm doing here. + The Ruby implementation does other stuff which I omit. + */ + if (Extend) { + String *wrap = Getattr(n, "wrap:code"); + if (wrap) { + Printv(f_wrappers, wrap, NIL); + } + } + + // HACK: this is only for the v8 emitter. maybe set an attribute wrap:action of node // TODO: generate dtors more similar to other wrappers + // EW: I think this is wrong. delete should only be used when new was used to create. If malloc was used, free needs to be used. if(SwigType_isarray(type)) { Printf(free, "delete [] (%s)", ctype); } else { Printf(free, "delete (%s)", ctype); } - state.clazz(DTOR, wrap_name); - t_dtor.replace(T_NAME_MANGLED, state.clazz(NAME_MANGLED)) + String* destructor_action = Getattr(n, "wrap:action"); + // Adapted from the JSCore implementation. + /* The next challenge is to generate the correct finalize function for JavaScriptCore to call. + Originally, it would use this fragment from javascriptcode.swg + %fragment ("JS_destructordefn", "templates") + %{ + void _wrap_${classname_mangled}_finalize(JSObjectRef thisObject) + { + SWIG_PRV_DATA* t = (SWIG_PRV_DATA*)JSObjectGetPrivate(thisObject); + if(t && t->swigCMemOwn) free ((${type}*)t->swigCObject); + if(t) free(t); + } + %} + + But for the above example case of %extend to define a destructor on a struct, we need to override the system to not call + free ((${type}*)t->swigCObject); + and substitute it with what the user has provided. + To solve this, I created a variation fragment called JS_destructoroverridedefn: + SWIG_PRV_DATA* t = (SWIG_PRV_DATA*)JSObjectGetPrivate(thisObject); + if(t && t->swigCMemOwn) { + ${type}* arg1 = (${type}*)t->swigCObject; + ${destructor_action} + } + if(t) free(t); + + Based on what I saw in the Lua and Ruby modules, I use Getattr(n, "wrap:action") + to decide if the user has a preferred destructor action. + Based on that, I decide which fragment to use. + And in the case of the custom action, I substitute that action in. + I noticed that destructor_action has the form + delete_MyData(arg1); + The explicit arg1 is a little funny, so I structured the fragment to create a temporary variable called arg1 to make the generation easier. + This might suggest this solution misunderstands a more complex case. + + Also, there is a problem where destructor_action is always true for me, even when not requesting %extend as above. + So this code doesn't actually quite work as I expect. The end result is that the code still works because + destructor_action calls free like the original template. The one caveat is the string in destructor_action casts to char* which is wierd. + I think there is a deeper underlying SWIG issue because I don't think it should be char*. However, it doesn't really matter for free. + + Maybe the fix for the destructor_action always true problem is that this is supposed to be embedded in the if(Extend) block above. + But I don't fully understand the conditions of any of these things, and since it works for the moment, I don't want to break more stuff. + */ + if(destructor_action) { + Template t_dtor = getTemplate("js_dtoroverride"); + state.clazz(DTOR, wrap_name); + t_dtor.replace("${classname_mangled}", state.clazz(NAME_MANGLED)) + .replace(T_WRAPPER, wrap_name) + .replace(T_FREE, free) + .replace(T_TYPE, ctype); + + t_dtor.replace("${destructor_action}", destructor_action); + Wrapper_pretty_print(t_dtor.str(), f_wrappers); + } + else { + Template t_dtor = getTemplate("js_dtor"); + state.clazz(DTOR, wrap_name); + t_dtor.replace(T_NAME_MANGLED, state.clazz(NAME_MANGLED)) .replace(T_WRAPPER, wrap_name) .replace(T_FREE, free) .replace(T_TYPE, ctype) .pretty_print(f_wrappers); + } Delete(p_classtype); Delete(ctype);