Re: Re: plua merged into lua

Hi, I'm unfortunately missing the time to go through this properly but to answer at least a bit more than I did on IRC a few comments: In php_lua.h line 90-110: This code is dependent on C99 and some compilers. Even when more code I prefer having the long version in the code instead of these non-portable shortcuts. Additionally one should use php_error_docref() this formats the error a bit nicer and can directly link to the documentation. Also: why is lua_error() using E_USER_ERROR not E_ERROR? The USER ones are meant to be used from userspace. In lua.c I do not understand why you need the LUA_G(instances) table. The individual instances should be stored in the object (as is lua_obj->L = L; ) and manged via that object. In lua.c I see that Lua::register seems to only accept global PHP functions as a callback. Is there no way to attach custom data to the lua closure, so we can store zend function pointers (as returned by the "f" modifier to zend_parse_parameters) so object methods and closures work, too? Related to that I also see the usage of a property _callbacks. If the information is needed it should probably stored in the object structure, this actually might be what is needed for what i said before. Secodly if the property is kept it is strange: The class Lua is marked as final, while the property is set to protected. This combination doesn't really make sense. Maybe it should be private. Secondly it is static, while callbacks are set by object instance. In lua.c php_lua_send_zval_to_lua's case IS_ARRAY should have a recursion protection. (see HashTable.nApplyCount, recursion is easy: $a = array(); $a[] &= $a; ) In lua.c php_lua_send_zval_to_lua I'm not sure about the array conversion. This will only work for objects making use of "normal" properties. It will cause trouble with private/protected properties (You get \0-bytes in the result). Maybe it makes sense to create an interface like this: interface LuaConvertable { public function getElements() } which returns a list of all properties and their name and value as they should be exported. If not one has to think about iterators and using get_property object handlers. In lua.c PHP_METHOD(lua, call) - why does this function exist? I think it should be an alias in the function table (PHP_MALIAS). I also think call should be the primary name and __call the alias to it. In lua.c in php_lua_get_zval_from_lua, if a TTABLE is returned and the key is neither numeric nor a string there should be a warning for the user. Maybe it also makes sense not to use key = php_lua_get_zval_from_lua(L, -2 TSRMLS_CC); but a specific handling routine. In lua.c proto Lua::getVersion() should be renamed from getversion to getVersion ;-) In lua.c Lua::register probably should be renamed to something like Lua::registerCallback. In lua.c all(?) PHP_METHODs: After a failure in zend_parse_parameters (zpp) do a simple "return;" not WRONG_PARAM_COUNT. zpp will already report an error, way more precise than WRONG_PARAM_COUNT. As an additional comment, not tooo relevant here: the php_{$extname}.h header should be short and contain little more than the phpext_{$extname}_ptr as this file is included from main/internal_functions.c along with all other extension's headers when doing a static build and therefore increases compile times and increases the risk of symbol conflicts (especially when including "random" library headers) which can be avoided. johannes On Thu, 2011-08-11 at 11:48 +0800, Laruence wrote: > Hi: > johanees, do you think it's the time to make a release now? > (the issues you metioned last time: version $Id, I will subtitute > it before release); > > > thanks > > 2011/8/6 Andreas Streichardt <andreas.streichardt@googlemail.com>: > > Hi! > > I just had a quick look at the code and it looks very good and way more > > professional than my code :) > > So i would say: release it. I will update a few links i set to my extension > > to the pecl homepage. > > Kind regards, > > Andreas Streichardt > > > > > > Laruence <laruence@php.net> hat geschrieben: > > > > > > Hi: > > if I can get your agreement before next sunday(14 Aug), > > I will make the first relase with version 1.0.. > > > > alos doc is in process. > > > > thanks > > > > 2011/8/4 Laruence <laruence@php.net>: > >> Hi: > >> > >> I have merged plua src into lua, add add package2.xm to build pecl > >> package. > >> > >> could you plz review my codes when you are free? > >> > >> thanks > >> > >> -- > >> Laruence Xinchen Hui > >> http://www.laruence.com/ > >> > > > > > > > > -- > > Laruence Xinchen Hui > > http://www.laruence.com/ > > > > >