Re: clusterize extension

Hi, Anatol Thank you for your review. I fixed these points: GPL'd extensions unfortunately are not accepted in PECL I've already changed license to PHP 3.01; arData should not be accessed directly, please check ZEND_HASH_FOREACH* > macro family in zend_hash.h I rewrote that part to use this macro family, thank you :) variable clusters_count is declared as zval, read_in as "l" from ZPP and > then casted to uint32_t. Likely it should be zend_long. Yes, you right. I changed it to zend_long; functions like kmeans_malloc could be patched to use Zend allocator, since > the file is bundled anyway > I changed that macro to emalloc for kmeans_malloc and efree for kmeans_free. But I'm not sure about including php.h to that header. config.w32 could be added > I added config.w32 and also I added integration with AppVeyor to check if it works on Windows; PHP's core sticks to C89. The code contains mixed declarations, one line > comments and some functions like lroundf, which are C99 actually. Probably > not critical nowadays, but beyond portability issues can be expected. > I changed all code to be valid with C89 standard. And replaced lroundf with floor – actually, it works pretty well too :) Ivan On Wed, May 9, 2018 at 9:48 AM, Anatol Belski <weltling@outlook.de> wrote: > Hi Ivan, > > > -----Original Message----- > > From: Ivan Pomortsev <ivan.pomortsev@gmail.com> > > Sent: Monday, May 7, 2018 12:47 AM > > To: pecl-dev@lists.php.net > > Subject: [PECL-DEV] clusterize extension > > > > Hello, > > I'm Ivan Pomortsev > > > > I want you to review my extension for clustering geo points on maps and > apply it > > to publish at pecl.php.net. > > Source code is available here: https://github.com/iworker/clusterize‡{Xâ > > ‘ðcmÍl4ó > > > Here's a quick review > > - GPL'd extensions unfortunately are not accepted in PECL > - arData should not be accessed directly, please check ZEND_HASH_FOREACH* > macro family in zend_hash.h > - variable clusters_count is declared as zval, read_in as "l" from ZPP and > then casted to uint32_t. Likely it should be zend_long. > - functions like kmeans_malloc could be patched to use Zend allocator, > since the file is bundled anyway > - config.w32 could be added > - PHP's core sticks to C89. The code contains mixed declarations, one line > comments and some functions like lroundf, which are C99 actually. Probably > not critical nowadays, but beyond portability issues can be expected. > > Regards > > Anatol >