Re: New extension 'sync'

On 5/5/2014 6:31 AM, Shuhei Tanuma (chobie) wrote:

2014-05-04 8:13 GMT+09:00 Thomas Hruska <thruska@cubiclesoft.com>:

I think I'm ready for a preliminary code review for PECL inclusion. I've been looking over the various documents on the account request page and I see a few potential issues, but I'd like feedback at this point for guidance/pointers/tips specific to actions I actually need to take vs. nice-to-haves before applying for a PECL account.

I've built your extension with php5.3 but it failed. You need to define zval *tmp here.

#if PHP_VERSION_ID < 50399
        zend_hash_copy(obj->std.properties, &ce->default_properties,
(copy_ctor_func_t)zval_add_ref, (void *)&tmp, sizeof(zval *));

Fixed. Regression was introduced while testing the Windows variant.

Sync_Mutex class name would be SyncMutex better. please check User Functions/Methods Naming Conventions #7. http://git.php.net/?p=php-src.git;a=blob_plain;f=CODING_STANDARDS;hb=HEAD

Changed.

Why do you choose multi licensing for php extension? Single license would be simpler.

Because user choice is what open source is about. More license options is therefore better. This is the first time anyone has complained about having a choice. -- Thomas Hruska CubicleSoft President I've got great, time saving software that you will find useful. http://cubiclesoft.com/