Re: Lowercase or camelcase function names?

Hi, On Mon, 2011-11-07 at 22:00 +0100, Johannes wrote: > > Some small comments, without really deep into it: > > * pval isn't used anymore, I think it was even dropped with 5.3. > > zval is the exact same. > > Fixed. No use of both of them anymore. I meant to say that "zval is the exact same thing as a pval was" (as a pval was a typename'd zval, historically pval was from PHP 3 times) zval can still be used (while zend_parse_paramters often makes it unnecessary). I did not mean to say "zval is the same as pval and should be dropped". > > * The PARSE_ARGS and ARG macros use an old dangerous, bad API. > > Look into using zend_parse_parameters (see > > README.PARAMETER_PARSING in php src) > > * The current code does ARG(0)->value.lval, this should be > > Z_LVAL_P(ARG(0)), while that call is, at least in > > scard_release_context, unsafe, zend_parse_parameters fixes that, > > though) > > Both items fixed. I have changed the parameter handling code to use > "zend_parse_parameters". Macros removed. small comment: It is a convention to do a simple "return;" and therefore return NULL if parameter parsing fails, not RETURN_FALSE. See note in http://php.net/functions.internal > > * Our CODING_STYLE asks for {} for every if clause etc. even when > > it's just a single line, the opening { should be on the same > > line as the "if" (or "while", "do", ..) > > Fixed. > > > * I assume there are some leaks, make sure to build PHP with > > --enable-debug > > Done... 4 Leaks are still open... ;) For a more complete check and more information on a Linux, BSD or Mac system you can use valgrind. $ USE_ZEND_ALLOC=0 valgrind --leak-check=full --show-reachable==yes /path/to/php script.php or $ /path/to/php run.tests.php -m (if you have phpt tests) See Zend/README.ZEND_MM for a bit more on that. :-) > > * The CREDITS file should be two lines, first line extension name, > > second line author names. This is afaik used by the Windows > > build. It would also be used to create phpcredits() if this > > extension was to be bundled with PHP (which I don't expect to > > happen) > > Done. > > > * build.shlooks weird, phpize && ./configure && make should do all > > the things needed, else config.m4 is wrong. > > Will check that. Just "phpize && ./configure && make" fails... What kind of error do you get? > > * php_pcsc.h should be as sparse as possible, as for a static > > build it is included from main/builtin_functions.h which causes > > conflicts. > > Cleaned it up... Am I missing something? * I think you don't have to include <PCSC/pcsclite.h> from there. * If you restructure the C file you could (I'd do it but it's not very important) get rid of the forward declarations for the PHP_FUNCTIONs and PHP_*_FUNCTIONs (for that,, in the C file, out the module entry last, and the function tbale right in front of it, after the actual function implementations) * defines like TRUE/FALSE should be as local as possible (in the .c file, if needed, Zend defiens them somewhere that should be enough...) From a technical perspective the only thing needed in that file would be #ifndef PHP_PCSC_H #define PHP_PCSC_H extern zend_module_entry pcsc_module_entry; #define phpext_pcsc_ptr &pcsc_module_entry #endif /* PHP_PCSC_H */ These two declarations are needed for a static build. If I need other stuff in a header (usually only needed when having multiple .c files) I try to keep the php_$extname.h one as short as the one shown above and use a $extname_internal.h one for the other things. But that's just my way. Not necessarily "the best". One thing I did not realize yesterday is that you're using the type "LONG". C only defines a type "long". Is LONG defined in pcsc? Will it be compatible to C's "long"? (C's long can be a bit "annoying" - on 32bit machines it is usually 32 bit. On 64bit Unix/Linux machines it usually is 64bits, on 64bit Windows it's 32bit) > Should I do an account request in such a early state of the extension? > I really would like to provide it ASAP but I don't know if you like > 0.1.0 versions at pecl.php.net. A good thing would be the use of PHP's > version control to keep changes at a central place... I think that's fine. You might give others one or two days for a chance to object, though. > P.S.: I need to replace all my whitespaces with tabs... :/ In PECL we aren't as strict as in PHP Core (where we aren't as strict as we could be, either ...) but the closer it is to the general style the simpler it becomes to get help ... from my perspective the key should be to at least be consistent, right now you're mixing tabs and space, but I assume you know. johannes