Re: Become a mantainer of wxphp

Hi, On Wed, 2012-05-02 at 13:56 -0400, JGM wrote: > [quote=Johannes]Did you talk to Mário when taking it over? Is he ok with > that? Is there > a "migration path" for people coming from the old version to the new > one? Will everything still work? (not needed, but useful ...)[/quote] > > I maintain contact with Mário, If IIRC I already talked about this with him > but I wanted to have something working and familiarize more with the zend > engine before becoming a maintainer. Old version of this extension didn't > supported a lot of things, and had bugs that I fixed. Also wxWidgets > interface is mostly consistent so if somebody used old version this new one > should work with little or no changes at all. Ok. Just want to make sure we don't go over somebody's head, that's not nice, usually. And having a migration path for users is always good. (even for new users, who are more likely to trust the new code ifthey don't have to fear it will break in future ;-) ) > [quote=Johannes]If PHP_METHOD(php_wxApp, Yield) in app.cpp is a > hand-written it is > strange (for instance it sets "valid=1" and then does "if (valid)"it > also does an "if (ZEND_NUM_ARGS()==0)" followed by a switch based on the > same value. Functions not expecting parameters should use > zend_parse_parameters_none() to produce a nicer error.[/quote] > > I haven't touched the wxApp wrapper much but I guess that Mário maybe used > parts of the code generator to create that class and then tweak it, so > maybe thats why it looks like that. If there's no reason to keep it like this one might clean it up. > [quote=Johannes]zend_parse_parameters is always set to quiet mode, > therefore the user > won't get a proper error when passing wrong arguments..[/quote] > > Thats due to the fact that wxWidgets class methods sometimes (almost all > the time :D) have different overloads, so the method can expect any number > of combinations of arguments, I don't know if theres a better way to handle > that situation. A typical pattern used sometimes is something like this: (I'm too lazy to type long names ;-) ) if (zpp_ex(QUIET, "sss", ...) == FAILURE) { if (zpp_ex(QUIET, "sa", ...) == FAILURE) { if (zpp("a", ...) == FAILURE) { return; } /* handle version with a single a */ } else { /* ....*/ One could also do switch (ZEND_NUM_ARGS()) { case 1: if (zpp() == FAILURE) return; case 2: if (zpp() == FAILURE) return; default: WRONG_PARAMETER_COUNT; return; } one key item is that there should always be an error message if it is wrong in some way. In the first case the last one will create an error (which can be a bit misleading ...) the second case can provide more specific errors in some cases, but gives the imprecise "wrong parameter" message. I don't know what is best in this case :-) > [quote=Johannes]Preprocessor macros should always start in the first > column.[/quote] > > Did some indentation to some macros that enclose debugging code because I > found it easier to read that starting the macro at first column then huge > indentation to see actual code inside the macro, so is mandatory to have it > of first column ? :) I just checked the C99 standard (don't have C89 or C++ at hand right now) and contrary to what I thought whitespaces are allowed before the #. I remember some trouble with older compilers ... but I guess the code is ok. > [quote=Johannes]You're using call_user_function_ex() the > zend_call_method[_with_[1-3]_param[s] family of functions from > zend_interfaces.h can be nicer.[/quote] > > Ahh thanks a lot for that one, I noticed that using call_user_function_ex > doesn't works well when calling a private method from a public virtual > method when working with classes. So that should fix it I guess :) Yes, zend_call_method accepts a class_entry as "scope" to identify which priate/protected elements can be accessed. > [quote=Johannes]Why are you using objects and resources?[/quote] > > Thats actually for historical reasons thats how it was coded originally, > and I'm still a noob with the zend engine, I have also asked that question > my self, why it was done that way, and use of "wxResource" string as push > objects as resources. But still I don't know which other way can be done. The basic pattern is to extend zend_object in the C way struct my_object { zend_object zo; int a_private_integer_not_accesible_from_php; my_data_type* custom_data; void* anything; } When doing this in C++ you have to ensure the zend_object is in the beginning of the structure and that the compiler doesn't put any metadata in front (in other words (size_t)&object == (size_t)&object->zo must be true), probably an extern "C" declaration is needed. Then add a C/C++-level constructor and destructor: void my_object_free(void *object TSRMLS_DC) { struct my_object *intern = (struct my_object*)object; free(intern->anything); zend_object_std_dtor(&intern->zo TSRMLS_CC); efree(intern); } zend_object_value my_object_new(zend_class_entry *class_type TSRMLS_DC) { zend_object_value retval; struct my_object* intern; intern = emalloc(sizeof(struct my_oject)); zend_object_std_init(&intern->zo, class_type TSRMLS_CC); object_properties_init(&intern->zo, class_type); intern->a_private_integer_not_accesible_from_php = 42; intern->anything = some_data; retval.handle = zend_objects_store_put(intern, NULL, my_object_free, NULL TSRMLS_CC); return retval; } Mind, again, they are called from C and might need extern "C" binding. Then register this with the class during MINIT: INIT_CLASS_ENTRY(my_class_entry, "my_class", my_functions); my_class_entry.create_object = my_object_new; my_class_entry = zend_register_internal_class(&my_class_entry TSRMLS_CC); And then, whenever you get the object you can cast from zend_object* to my_object* and access the private data. Check the PHP source and OO PECL exts for occurrences of"create_object" to find samples. > [quote=Johannes]I think bool wxLogWindow_php::OnFrameClose(wxFrame* frame) > won't freearguments[0] and [1] properly. Did you check for leaks? [/quote] > > I actually focused on code that handles destruction of objects when they > become out of scope, since on the older version this wasn't handle > resulting in a lot of memory leaks. So I have to check those that you > mention since I thing I introduced them trying to fix other issues X_X :-) johannes