Hello,
On Sat, Jan 17, 2009 at 9:10 PM, Marcus Boerger <[email protected]> wrote:
> Hello Etienne,
>
> Friday, January 16, 2009, 11:20:01 PM, you wrote:
>
>> colder Fri Jan 16 22:20:01 2009 UTC
>
>> Modified files:
>> /php-src/ext/spl spl_observer.c
>> Log:
>> Implement SplObjectStorage::addAll/removeAll
>>
>> http://cvs.php.net/viewvc.cgi/php-src/ext/spl/spl_observer.c?r1=1.31&r2=1.32&diff_format=u
>> Index: php-src/ext/spl/spl_observer.c
>> diff -u php-src/ext/spl/spl_observer.c:1.31
>> php-src/ext/spl/spl_observer.c:1.32
>> --- php-src/ext/spl/spl_observer.c:1.31 Wed Jan 14 15:51:54 2009
>> +++ php-src/ext/spl/spl_observer.c Fri Jan 16 22:20:00 2009
>> @@ -13,10 +13,11 @@
>> | [email protected] so we can mail you a copy immediately.
>>
>> +----------------------------------------------------------------------+
>> | Authors: Marcus Boerger <[email protected]>
>> + | Etienne Kneuss <[email protected]>
>>
>> +----------------------------------------------------------------------+
>> */
>>
>> -/* $Id: spl_observer.c,v 1.31 2009/01/14 15:51:54 colder Exp $ */
>> +/* $Id: spl_observer.c,v 1.32 2009/01/16 22:20:00 colder Exp $ */
>>
>> #ifdef HAVE_CONFIG_H
>> # include "config.h"
>> @@ -332,6 +333,62 @@
>> }
>> } /* }}} */
>>
>> +/* {{{ proto bool SplObjectStorage::addAll(SplObjectStorage $os)
>> + Add all elements contained in $os */
>> +SPL_METHOD(SplObjectStorage, addAll)
>
> Can we name this copy(), to me addAll() is a pretty bad choice for what
> you do. Maybe we also want to add copyFromIterator() and copyFromArray()
>
Mmmh, I thought is was a somewhat valid name, i.e. java's collection
API uses addAll(Collection $o)
The only problem I saw with arrays and iterators is that you can't
have object => info map with both, since both array and iterator keys
can't be objects.
I thought about it and IMO we can always relax the argument type
restriction and allow arrays and iterators if we feel the need,
instead of extending the API.
>> +{
>> + zval *obj;
>> + spl_SplObjectStorage *intern = (spl_SplObjectStorage
>> *)zend_object_store_get_object(getThis() TSRMLS_CC);
>> + spl_SplObjectStorage *other;
>> + spl_SplObjectStorageElement *element;
>> + HashPosition pos;
>> +
>> + if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "O", &obj,
>> spl_ce_SplObjectStorage) == FAILURE) {
>> + return;
>> + }
>> +
>> + other = (spl_SplObjectStorage *)zend_object_store_get_object(obj
>> TSRMLS_CC);
>> +
>> + zend_hash_internal_pointer_reset_ex(&other->storage, &pos);
>> + while (zend_hash_get_current_data_ex(&other->storage, (void
>> **)&element, &pos) == SUCCESS) {
>> + spl_object_storage_attach(intern, element->obj, element->inf
>> TSRMLS_CC);
>> + zend_hash_move_forward_ex(&other->storage, &pos);
>> + }
>> +
>> + zend_hash_internal_pointer_reset_ex(&intern->storage, &intern->pos);
>> + intern->index = 0;
>> +
>> + RETURN_LONG(zend_hash_num_elements(&intern->storage));
>> +} /* }}} */
>> +
>> +/* {{{ proto bool SplObjectStorage::removeAll(SplObjectStorage $os)
>> + Remove all elements contained in $os */
>> +SPL_METHOD(SplObjectStorage, removeAll)
>
> How about, excessDelete() or even better removeOverlap() because
> removeAll() suggests a completely different opeartion.
Since it basically is the opposite of addAll, removeAll seemed
adequate... Of course removeAll without argument is basically a
clear(), but with the argument it seems to me like it suggests what it
does...
IMO we should keep some kind of naming similarity between the two
methods since they are closely related.
Is it the "All" that is bothering you ? Since we already have
attach/detach, what do you think of attachAll and detachAll ?
Regards
>
>> +{
>> + zval *obj;
>> + spl_SplObjectStorage *intern = (spl_SplObjectStorage
>> *)zend_object_store_get_object(getThis() TSRMLS_CC);
>> + spl_SplObjectStorage *other;
>> + spl_SplObjectStorageElement *element;
>> + HashPosition pos;
>> +
>> + if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "O", &obj,
>> spl_ce_SplObjectStorage) == FAILURE) {
>> + return;
>> + }
>> +
>> + other = (spl_SplObjectStorage *)zend_object_store_get_object(obj
>> TSRMLS_CC);
>> +
>> + zend_hash_internal_pointer_reset_ex(&other->storage, &pos);
>> + while (zend_hash_get_current_data_ex(&other->storage, (void
>> **)&element, &pos) == SUCCESS) {
>> + spl_object_storage_detach(intern, element->obj TSRMLS_CC);
>> + zend_hash_move_forward_ex(&other->storage, &pos);
>> + }
>> +
>> + zend_hash_internal_pointer_reset_ex(&intern->storage, &intern->pos);
>> + intern->index = 0;
>> +
>> + RETURN_LONG(zend_hash_num_elements(&intern->storage));
>> +} /* }}} */
>> +
>> /* {{{ proto bool SplObjectStorage::contains($obj) U
>> Determine whethe an object is contained in the storage */
>> SPL_METHOD(SplObjectStorage, contains)
>> @@ -907,6 +964,8 @@
>> SPL_ME(SplObjectStorage, attach, arginfo_attach, 0)
>> SPL_ME(SplObjectStorage, detach, arginfo_Object, 0)
>> SPL_ME(SplObjectStorage, contains, arginfo_Object, 0)
>> + SPL_ME(SplObjectStorage, addAll, arginfo_Object, 0)
>> + SPL_ME(SplObjectStorage, removeAll, arginfo_Object, 0)
>> SPL_ME(SplObjectStorage, getInfo, NULL, 0)
>> SPL_ME(SplObjectStorage, setInfo, arginfo_setInfo, 0)
>> /* Countable */
>
>
>
>
>
>
> Best regards,
> Marcus
>
>
>
>
--
Etienne Kneuss
http://www.colder.ch
Men never do evil so completely and cheerfully as
when they do it from a religious conviction.
-- Pascal
--
PHP CVS Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php
