Re: Re: plua merged into lua

Hi: Wow, thanks very much, johannes, these comments are so useful, and I am very appreciate it. I will fixed these issues soon, after that, sure, a beta version should be released first. also there are some question you asked , I answer them blow the question~ thanks 2011/8/16 Johannes Schlüter <johannes@schlueters.de>: > 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. Doing this , is because I need to free a Resource when it was unset, and there must be a way to find out whether a Lua instance is valid, and since this is a Extension ineternal mechanism, so I prefer to use LUA_G > > 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? yes, but "f" is introduced after php 5.3. maybe I should use some macro tricky, and in fact array(obj, method) is accpected now, ;-) > 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/ >> > >> >> >> > > > -- Laruence  Xinchen Hui http://www.laruence.com/