On Sun, Feb 16, 2014 at 4:11 PM, Johannes Schlüter <[email protected]> wrote:
> 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 > > > based on the lack of response from the author, I'm going to reject the account request for now. -- Ferenc Kovács @Tyr43l - http://tyrael.hu
