Page MenuHomePhabricator

eina: introduce Eina_Abstract_Content
AcceptedPublic

Authored by bu5hm4n on Jan 5 2020, 6:06 AM.

Details

Summary

A little abstraction to have abstract data content bound to a type.

Depends on D11062

Diff Detail

Repository
rEFL core/efl
Branch
devs/bu5hm4n/work_cnp
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 16033
There are a very large number of changes, so older changes are hidden. Show Older Changes
bu5hm4n requested review of this revision.Jan 5 2020, 6:07 AM

This is for now just a draft, more conversions are missing, and i think we should be able to pass a content with a type as Eina_Value. However, i think feedback here could be of interest.

bu5hm4n updated this revision to Diff 28034.Jan 8 2020, 1:01 PM

add eina_value implementation

cedric requested changes to this revision.Jan 8 2020, 3:38 PM
cedric added inline comments.
src/lib/eina/eina_abstract_content.c
78

I would prefer the use of a goto and only one eina_stringshare_del in this function.

122

This is really a hack. Why are you not just returning: eina_list_iterator_new(possibilities).

140

Maybe it would be best to do something about this :-)

175

There is no close done on eina side. It should be properly documented. Also, from an API point of view, this isn't great as their is both a risk of leaking the fd and their could be a race condition in reopening the file instead of using a fd. Considering my comment above, it would actually be best to just have an Eina_File API.

283

This isn't great. Having a minimum support for conversion to and from STRING, BLOB, FILE would make things more usable.

350

The callbacks hash should be destroyed here.

354

I do not understand why it is a double pointer here.

src/lib/eina/eina_abstract_content.h
3

I think you should choose either Eina_Content or Eina_Abstract_Content.

37

You could introduce easily an: EAPI Eina_File *eina_content_as_mmap(Eina_Content *content); that could be quite useful and work in all case (Even when the content is not in a file, you can still map an Eina_Slice to a virtual Eina_File).

67

Is it on purpose that it is not an Eina_Slice *?

94

I think register should be at the end of the function name.

src/tests/eina/eina_test_abstract_content.c
30

This is a good example of the problem of the API as it is leaking the fd and actually double opening the file.

This revision now requires changes to proceed.Jan 8 2020, 3:38 PM
bu5hm4n planned changes to this revision.Jan 8 2020, 11:57 PM
bu5hm4n added inline comments.
src/lib/eina/eina_abstract_content.c
78

sure

122

I dont want to publish Eina_Content_Conversion_Node as a struct. Which would mess up the iterator. (Pointer to a Pointer of a String)

140

Just replacing the last character ? (I am somewhat against resizing the whole thing, thats a little bit of a too big overhead). Or simply return NULL...?

175

I forgot the close ... :)

The API is not meant to return a Eina_File. This is just there to have a path fast at the hand that you can continue to use. (Like _tempfile_new in efl_ui_selection_manager.c). If someone wants to open the file again from the same process as this API is called, then he is doing something weird, he can just read the slice.

283

I went a lot back and forth with this. My problem is, i would only allow STRING conversion if its a utf8 string, which would only be a few cases, BLOB is the problem of "we don't know what it is" so i cannot think of a type for it. And File to a text/plain path would be possible, but there is the question of why you would do that.

If something shows up in future that we should have it, we still can add it, but i dont want to provide a swiss army knife that is not used at all :/

354

I forgot *value = NULL at the end of the function. I want it to be cleaned up because this here is freeing the content.

src/tests/eina/eina_test_abstract_content.c
30

As I said before, the leaking fd is a error I made, the reason this here is double opening the file is that i actually want to test if the content does match the slice, which is nothing you will ever do as API user ... :)

bu5hm4n updated this revision to Diff 28068.Jan 9 2020, 1:07 PM

Apply planned changes.

cedric requested changes to this revision.Jan 9 2020, 1:59 PM

Some more review.

src/lib/eina/eina_abstract_content.c
14

Please use EINA_REFCOUNT.

122

Hum, this is not the only case we have for this. Maybe a cleaner solution would be to introduce a new iterator that take an iterator and an offset to return the pointer at that offset. Something like: eina_iterator_offset_pointer_new(Eina_Iterator *source, int offset);. You would use it here like: it = eina_iterator_offset_pointer_new(eina_list_iterator_new(possibilities), offsetof(Eina_Content_Convertion_Node, to));.

140

I think returning NULL in this case is acceptable. It means error to the caller anyway.

175

I see. I am ok with the close this way.

This revision now requires changes to proceed.Jan 9 2020, 1:59 PM
bu5hm4n planned changes to this revision.Jan 10 2020, 2:22 AM
bu5hm4n marked 9 inline comments as done.
bu5hm4n added inline comments.
src/lib/eina/eina_abstract_content.c
14

Why do we have that, and why is it *nooooooo where* used :|

122

Or just eina_iterator_map(iterator, map_func) which calls a callback where you can "transform" the passed value (each of them), (sounds a little better in terms of utilisation such a problem).

bu5hm4n updated this revision to Diff 28078.Jan 10 2020, 6:26 AM
bu5hm4n marked 5 inline comments as done.

more changes

bu5hm4n updated this revision to Diff 28081.Jan 10 2020, 7:38 AM
bu5hm4n retitled this revision from Introduce Eina_Abstract_Content to eina: introduce Eina_Abstract_Content.
bu5hm4n edited the summary of this revision. (Show Details)

update

bu5hm4n updated this revision to Diff 28083.Jan 10 2020, 8:02 AM

forgotten changes

bu5hm4n updated this revision to Diff 28503.Sun, Jan 26, 3:35 AM

phab - behave!

bu5hm4n updated this revision to Diff 28515.Mon, Jan 27, 1:55 AM

fixes & more docs

segfaultxavi requested changes to this revision.Mon, Jan 27, 3:00 AM
segfaultxavi added inline comments.
src/lib/eina/eina_abstract_content.h
27

typo: Conversion

Don't you have a spell checker on your editor?

This revision now requires changes to proceed.Mon, Jan 27, 3:00 AM

Just answer these questions so I can update the docs, thx!

src/lib/eina/eina_abstract_content.c
416

Shouldn't the parameter be called content?

421

I'm not following this. Does eina_value_pset copy the content?
Are we freeing the EIna_Content that was provided to us?

427

This is equivalent to eina_value_content_new but returns an obj instead of a ptr, right?

bu5hm4n updated this revision to Diff 28525.Mon, Jan 27, 4:32 AM
bu5hm4n marked 11 inline comments as done.

fix api names

bu5hm4n updated this revision to Diff 28527.Mon, Jan 27, 5:31 AM

fix impl

cedric accepted this revision.Fri, Jan 31, 10:05 AM

Except for the minor cosmetic of the .h file name not matching the function name, I am good with this. You might want to add a _unregister function as some of the register could be done in a module and disappear at any point.

bu5hm4n updated this revision to Diff 28971.Wed, Feb 12, 5:51 AM

More fixes and progress

segfaultxavi requested changes to this revision.Wed, Feb 12, 7:05 AM
segfaultxavi added inline comments.
src/lib/eina/eina_abstract_content.c
35

Typo: Convertion

This revision now requires changes to proceed.Wed, Feb 12, 7:05 AM
bu5hm4n updated this revision to Diff 28993.Wed, Feb 12, 11:41 AM

naming fixes

segfaultxavi accepted this revision.Thu, Feb 13, 12:23 AM

No more questions, Your Honor.

This revision is now accepted and ready to land.Thu, Feb 13, 12:23 AM
bu5hm4n updated this revision to Diff 29071.Sat, Feb 15, 12:52 PM

fixes & rebase