Re: cvs: php-src /ext/spl spl_observer.c

Hello, On Sat, Jan 17, 2009 at 9:10 PM, Marcus Boerger <helly@php.net> 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 @@ >> | license@php.net so we can mail you a copy immediately. >> >> +----------------------------------------------------------------------+ >> | Authors: Marcus Boerger <helly@php.net> >> + | Etienne Kneuss <colder@php.net> >> >> +----------------------------------------------------------------------+ >> */ >> >> -/* $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