Hi Ivan,

> -----Original Message-----
> From: Ivan Pomortsev <ivan.pomort...@gmail.com>
> Sent: Saturday, May 12, 2018 3:13 PM
> To: Anatol Belski <weltl...@outlook.de>
> Cc: pecl-dev@lists.php.net
> Subject: Re: [PECL-DEV] 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 :)
> 
For nNumOfElements there's also an API  as zend_hash_num_elements(). In 
general, it is better to use APIs as they hide implementation details which 
might change at some point while APIs are more likely to be kept compatible.

> 
>       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.
> 
If a file has been bundled and its license allows it, so it could be patched. 
One can carry the patch within the extension, if needed, in case the bundled 
file needs to be updated, etc. In this exact case, the advantages of emalloc 
vs. malloc and et cetera is, that ZMM is used with better performance and 
correct memory usage calculation.

> 
> 
>       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 :)
> 
Cool, thanks for checking the notes. It's now to see for perhaps more comment 
and otherwise PECL admins to accept your request.

Regards

Anatol 

> 
> Ivan
> 
> On Wed, May 9, 2018 at 9:48 AM, Anatol Belski <weltl...@outlook.de
> <mailto:weltl...@outlook.de> > wrote:
> 
> 
>       Hi Ivan,
> 
>       > -----Original Message-----
>       > From: Ivan Pomortsev <ivan.pomort...@gmail.com
> <mailto:ivan.pomort...@gmail.com> >
>       > Sent: Monday, May 7, 2018 12:47 AM
>       > To: pecl-dev@lists.php.net <mailto: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 <http://pecl.php.net> .
>       > Source code is available here: https://github.com/iworker/clusterize
> <https://github.com/iworker/clusterize>
>       >
>       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
> 
> 

Reply via email to