Page MenuHomePhabricator

eina: introduce eina_iterator_process
ClosedPublic

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

Details

Summary

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

Depends on D11196

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bu5hm4n created this revision.Jan 10 2020, 7:37 AM
bu5hm4n requested review of this revision.Jan 10 2020, 7:37 AM
bu5hm4n updated this revision to Diff 28082.Jan 10 2020, 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.Jan 10 2020, 9:58 AM
This revision now requires changes to proceed.Jan 10 2020, 9:58 AM
cedric added inline comments.Jan 10 2020, 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.Jan 10 2020, 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.Jan 10 2020, 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.Jan 17 2020, 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.

bu5hm4n updated this revision to Diff 28502.Jan 26 2020, 3:35 AM

phab - behave!

bu5hm4n updated this revision to Diff 28514.Jan 27 2020, 1:55 AM
bu5hm4n edited the summary of this revision. (Show Details)

fixes & more docs

cedric accepted this revision.Jan 31 2020, 9:59 AM
This revision is now accepted and ready to land.Jan 31 2020, 9:59 AM
bu5hm4n updated this revision to Diff 28970.Feb 12 2020, 5:50 AM

More fixes and progress

bu5hm4n updated this revision to Diff 29070.Feb 15 2020, 12:51 PM

fixes & rebase

Closed by commit rEFL74491e8781d8: eina: introduce eina_iterator_process (authored by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyFeb 19 2020, 7:35 AM
This revision was automatically updated to reflect the committed changes.