Re: Uploading phpkmp extension to pecl

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

Attachment: [application/pgp-signature] This is a digitally signed message part signature.asc