Re: Uploading phpkmp extension to pecl

Thanks for reviewing the code! I will update the code this weekend Johannes Schlüter <johannes@schlueters.de> 於 2016年4月21日 星期四寫道： > Hi, > > On Tue, 2016-04-19 at 00:38 +0800, Lin Yo-An wrote: > > Hi all, > > > > > > I'd like to upload phpkmp extension (https://github.com/c9s/phpkmp) to > pecl. > > > A few comments: > > * As I understand the code kmp.c is supposed to be a stand-alone > library. There is a kmp_prefix_new() function, but no > kmp_prefix_free() function. A consumer has to know what to free > manually and be able to use the correct free(). A utility > function makes this simpler. In the new function you also don't > check whether malloc or calloc return NULL. (this check isn't > needed with emalloc as emalloc will terminate the script when > alloc fails) > * In PHP_FUNCTION(kmp_prefix) you essentially copy the code of the > mentioned kmp_prefix_new(). It might be useful to unify those by > giving a way to configure the allocator (emalloc vs .malloc) > * You have a function called F. This pollutes the global > namespace. That should be renamed. > * I (personally) don't like resources. Objects are nicer for the > user and often more efficient. Looking a bit closer I assume > that kmp_prefix()+kmp_search_prefix() doesn't provide notable > benefits over using kmp_search() always. The resource handling > will cost more than the loop in F() (but that's a feeling, a > simple benchmark could tell) > * You return FALSE if parameter parsing fails. We prefer returning > NULL. See > > > https://wiki.php.net/internals/review_comments#if_zend_parse_parameters_fails_do_a_simple_return > * I believe there will be an illegal write to P[1] in F() if a > length of 0 or 1 is used. (i.e. kmp_pefix("") or kmp_prefix("a") > ). I haven't checked the algorithm further. > * You might want to clean up your php_kmp.h header. This isn't > strictly needed, but nicer > > > https://wiki.php.net/internals/review_comments#php_extnameh_should_be_minimal > * It is preferred to have license headers in all files. > > johannes > -- Sent from Gmail Mobile