Hi Chen, On Tue, 2013-01-22 at 19:52 +0100, Ferenc Kovacs wrote: > >> hopefully somebody will take a look in the code, > >> in the meanwhile you can look into the following resources: > >> http://git.php.net/?p=php-src.git;a=blob_plain;f=CODING_STANDARDS;hb=HEAD > >> https://wiki.php.net/internals/review_comments > >> > >> > > > just bumping this so hopefully we won't forget reviewing this. >
I had a quite quick look over the code in https://github.com/chenyuduan/apns and here are a few (minor) comments: 1) php_apns.h has relatively much stuff in it. That isn't a problem per se but we try to keep those small. As that could be an issue for people uilding PHP statically. See https://wiki.php.net/internals/review_comments#php_extnameh_should_be_minimal 2) SSL *ssl; Global variables are bad - they cause trouble in threaded environments, better store it in the object. This specific case here will also likely cause issues if the user calls send() before connect(). In this case I think connect should be the constructor and send() a non-static member function. for storing a pointer in an object see http://talks.somabo.de/200903_montreal_php_extension_writing.pdf slide 89 and following. 3) typedef int bool; This is confusing to readers (and an int is quite large for a flag). We have zend_bool and the constants TRUE and FALSE. 4) string_to_bytes() in apns::send It is never checked that token bytes is large enough if input from is too much this will write in invalid memory. 5) const char * x = NULL; and const char * y = NULL; Those are passed into zend_parse_parameters which will write. which is invalid with the cosnt modifier. 6) char tokenbytes[32]; This should not be in the function body but aside the variable declarations. https://wiki.php.net/internals/review_comments#don_t_use_c99_for_portability_reasons 7) Empty R[INIT|SHUTDOWN] and empty functions table Those should be removed and the entries in the module entry should be set to NULL, this is a minimal improvement of performance. 8) char binarymessagebuff[l]; such dynamic array initialisation is a C99 feature, not available everywhere, better use a preprocessor #define to calculate the value. 9) License Current version of the license is 3.01, the code refers to 3.0. Clause 6 slightly changed http://www.php.net/license/3_0.txt vs http://www.php.net/license/3_01.txt This is a relatively long list but the mentioned items are mostly relatively small. There is a thing I wonder about though: This this require an extension? php streams or curl comined with pack() should be able to do all the things needed from userland while making it simpler to develop, test, deploy ... the code. Given that this is doing network operations incl. SSL handshakes the performance difference can be neglected. johannes -- PECL development discussion Mailing List (http://pecl.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
