Page MenuHomePhabricator

eina: introduce eina_iterator_process
Needs ReviewPublic

Authored by bu5hm4n on Fri, Jan 10, 7:37 AM.

Details

Summary

this brings a functional-map function to the iterator implementations.
https://en.wikipedia.org/wiki/Map_(higher-order_function)

Diff Detail

Repository
rEFL core/efl
Branch
devs/bu5hm4n/work_cnp
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 15328
bu5hm4n created this revision.Fri, Jan 10, 7:37 AM
bu5hm4n requested review of this revision.Fri, Jan 10, 7:37 AM
bu5hm4n updated this revision to Diff 28082.Fri, Jan 10, 8:01 AM

forgotten changes

Can you update the commit message also to remove the last ) that goes automatically in the url?

And I am quite baffle that this pass its own test, very weird.

src/lib/eina/eina_iterator.c
448

This call doesn't look great as it doesn't give a chance for the callback to stop the iterator. I think going with an API closer to the next callback: Eina_Bool process(Eina_Iterator *original, void **data, void *fdata); would be better and more flexible.

Also it does feel weird that we implicitly use data for both the internal iterator and the processed result. I would prefer them to be split especially with the change above.

And maybe, instead of original, should we pass the iterator container? When reading the callback doc, I kind of expected that.

469

Where does iterator go?

src/lib/eina/eina_iterator.h
384

So it is a map function, but we name the iterator processed... Why not naming it eina_iterator_map_new in this case?

Also it would be nice to name data, fdata actually to reduce potential confusion with the callback data.

cedric requested changes to this revision.Fri, Jan 10, 9:58 AM
This revision now requires changes to proceed.Fri, Jan 10, 9:58 AM
cedric added inline comments.Fri, Jan 10, 9:59 AM
src/tests/eina/eina_test_iterator.c
685

You should check that the iterator did actually iterate the right amount of time.

bu5hm4n updated this revision to Diff 28085.Fri, Jan 10, 10:43 AM
bu5hm4n marked 3 inline comments as done.
bu5hm4n edited the summary of this revision. (Show Details)

fix checks

bu5hm4n added inline comments.Fri, Jan 10, 10:57 AM
src/lib/eina/eina_iterator.c
448

You may want to step the iterator for different purposes:

  • Error on parsing some node -> We do not have a concept of errors in a iterator, that means a EINA_FALSE as a result would result in the iterator beeing stopped, which sounds quite "random"
  • You want to filter something -> use eina_iterator_filter_new

Thus i do not want to have the possibility to return anything from the process callback.

I decided for passing the original iterator, as you might be able to call there more things on when you get the container from the original container, (maybe, i dont know if there will be ever the case, but the processing iterator container is not really helpfull when it comes to that).

And the data field, well i implemented it like this, i dont see a problem with it ?

src/lib/eina/eina_iterator.h
384

The name was decided, because map is already used in the eina context for mmap things, and i want to avoid any confusion with that.

cedric added inline comments.Fri, Jan 17, 11:06 AM
src/lib/eina/eina_iterator.c
448

I think that limiting the ability of the iterator to not implement filter is going to bite us back later on. Chaining iterator, even if reducing code, imply more memory allocation and callbacks, which means slower operation. I would think that returning an Eina_Bool will allow to implement a filter (by just moving on to the next item in the iterator and calling the callback again).

Passing the parent iterator, kind of enable to do a filter, with most likely some more complex and painful code to write. Also please change the prototype of the callback. If we are passing an Iterator, it should be of iterator type.