Page MenuHomePhabricator

eo: here comes reflection API
ClosedPublic

Authored by bu5hm4n on Feb 5 2019, 7:13 AM.

Details

Summary

this adds support in eo to generate a reflection API. To get the actaul
reflection to the klass, the API efl_class_reflection_table_set needs to
be called, the table in the end can be generated by eolian. Reflection
API is inherited by the extended class. This means, if you have two
reflection tables, first, the most upperst is called, then the next
lower one is called.

For now this API accepts NULL setter or getter, and will ignore them
silently when they are called.

fix T7681

Depends on D7882

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.Feb 5 2019, 7:13 AM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

bu5hm4n requested review of this revision.Feb 5 2019, 7:13 AM
bu5hm4n added a child revision: D7880: eo: remove whitespaces.
cedric requested changes to this revision.Feb 5 2019, 9:25 AM

I am wondering if it wouldn't be better to modify efl_class_functions_set to take another parameter being the reflection array. This would solve the issue of the parent array not being set when initializing the child class and so enable the possibility to build the entire array for the class and all its inheritance in one go.

src/lib/eo/Eo.h
2011

I think it would be best to go with Eina_Value value, no pointer. And be explicit that the Eina_Value given will be flushed by the call to efl_property_reflection_set. This is how Eina_Future does and it simplify life I think as once you give an Eina_Value you do not need to care about it anymore.

2023

I think I would also return an Eina_Value, no pointer and make the caller responsible to flush the returned Eina_Value.

src/lib/eo/eo.c
3628

Wouldn't the following be nicer:

if (!klass->parent) return NULL;
return _efl_class_reflection_find(klass->parent, property_name);

I am also wondering if that is enough as we could end up having property defined in mixin and friends which wouldn't be explored with this code. Wouldn't it make sense to build a table with all the parent property so that we do not have to walk anything else but the class table? I understand this create trouble as this mean you can only build the table once at the initialization of the class.

This revision now requires changes to proceed.Feb 5 2019, 9:25 AM
bu5hm4n added inline comments.Feb 5 2019, 9:47 AM
src/lib/eo/Eo.h
2023

The reason this is a pointer is that i can return NULL when there is no getter.

src/lib/eo/eo.c
3628

The reason i don't want to assemble a list for this is, that we would have to have this list in memory and rw, which will leave it in RAM which would raise the memory consumtion, I want to work arround that. But i agree with you here, i should walk to whole tree, not just the parent.

cedric added inline comments.Feb 5 2019, 9:55 AM
src/lib/eo/Eo.h
2023

Wouldn't it make more sense to return an Eina_Value containing an Eina_Error in this case?

src/lib/eo/eo.c
3628

I think this is the wrong trade off. We do have few class in memory, but a lot of call on each object. It would seems we would want to be more efficient on the call and be ok with some memory duplication.

bu5hm4n marked 4 inline comments as done.Feb 5 2019, 10:01 AM
bu5hm4n updated this revision to Diff 19202.Feb 5 2019, 10:02 AM
bu5hm4n edited the summary of this revision. (Show Details)

update

bu5hm4n added inline comments.Feb 5 2019, 10:03 AM
src/lib/eo/Eo.h
2023

I continued with a empty for now, as no getter setter is a NOP for now, as there is no way to check setter / getter.

bu5hm4n added inline comments.Feb 5 2019, 10:12 AM
src/lib/eo/eo.c
3628

We do have a lot of calls, but searching a list with a strings as a key seems to be the more expensive part than just walking the tree.

The reason i want to avoid that is, just think of a single property like parent in efl_object.eo, this would mean every single class would contain a lot of allocated memory.

What i would do instead is, get a linear walk which walks up the tree element of a class of a object, which would resolve the issues of recursion there, while still just searching the list (IMO) :)

I think keeping it in a seperated API is okayish for now, as otherwise we would have a giant patch updating eolian / eolian-tests / eolian_cxx + eolian_cxx tests + eolian_mono tests etc. and i don't want to touch all of that right now, is it a deal breaker to have it in that api for now ?

I think keeping it in a seperated API is okayish for now, as otherwise we would have a giant patch updating eolian / eolian-tests / eolian_cxx + eolian_cxx tests + eolian_mono tests etc. and i don't want to touch all of that right now, is it a deal breaker to have it in that api for now ?

It would be best to make it part of the existing API and you can have multiple patch to do so. One would add a void * parameter to efl_class_functions_set and update all eolian and friends to just set it to NULL. Another patch will be this one, that define and document that new parameter. Finally another patch that would modify eolian and friends to generate it.

I do understand why you wouldn't want to take that road. An alternative that I would think acceptable is to enforce call to efl_class_reflection_table_set to be accepted only during the initializer callback during the class construction. Not perfect, but would avoid problems.

src/lib/eo/Eo.h
2023

I like the idea of returning an error code slightly better after thinking about it overnight. We could return error that indicate READ ONLY and WRITE ONLY. It also let the conversion code generate an error and make the error handler that receive the Eina_Value a single code path.

I won't have a stronger opinion than this. If you want to keep returning NULL, that is I think ok. Will have to play with the API and see.

src/lib/eo/eo.c
3628

I think that you are actually making a very good case for having one array. This enable us to reduce the call to strcmp to log(n) call by sorting the array and doing a binary search. If we had multiple array, we would loose that gain has it will be split among multiple smaller array with the addition of a call cost to the function that does the recursion.

We can implement later some memory optimization for object that have no property and just repeat their parent properties by having a refcount on the structure. Should reduce the footprint. Still the cost should only be for the class used at runtime by the program, I don't think we would have tons of them, compared to the potential cost of doing the search+call.

bu5hm4n updated this revision to Diff 19209.Feb 6 2019, 1:14 AM
bu5hm4n edited the summary of this revision. (Show Details)

update

cedric accepted this revision.Feb 6 2019, 2:02 AM
cedric added inline comments.
src/lib/eo/Eo.h
901

Just a potential coherency issue here as the rest of the API take an array plus its length instead of a NULL terminated one. I have no big feeling about it. So you decide and can land this version or an Efl_Object_Property_Reflection_Ops.

This revision is now accepted and ready to land.Feb 6 2019, 2:02 AM
bu5hm4n added inline comments.Feb 6 2019, 3:02 AM
src/lib/eo/Eo.h
901

True i will add it like this :)

segfaultxavi requested changes to this revision.Feb 6 2019, 3:10 AM
segfaultxavi added a subscriber: segfaultxavi.

Doc Cop is watching you!

src/lib/eo/Eo.h
864

Please add Doxygen docs (/** */ and /**< */) to all these typedefs, structs and fields.

869

The function used to set a generic #Eina_Value on this property of the object.

870

The function used to retrieve a generic #Eina_Value from this property of the object.

898

Use efl_property_reflection_set() (with the parenthesis at the end) so Doxygen creates automatic links. Same for _get() and the rest of methods you reference below.

901

I do have a strong feeling. Consistency is very important :)

Moreover, where in the docs does it say that the array must be NULL-terminated?

1984

Set the given #Eina_Value to the property with the specified \c property_name.

1986

The name of the property to modify.

1995

Retrieve an #Eina_Value containing the current value of the property specified with \c property_name.

1997

The name of the property to modify.

This revision now requires changes to proceed.Feb 6 2019, 3:10 AM
bu5hm4n updated this revision to Diff 19210.Feb 6 2019, 5:25 AM

doc updates

The Documents are updated, I wanted to add a test for mixins / interfaces which will be added in a follow up commit, since this requires us to have a little refactor in the eo test suite, as we don't have the possibility of just simply adding a mixin without copy pasting things arround, so this will follow up.

bu5hm4n marked 13 inline comments as done.Feb 6 2019, 5:35 AM
segfaultxavi accepted this revision.Feb 6 2019, 5:38 AM

Nicely done!
Although there are still some function names missing the () at the end...

This revision is now accepted and ready to land.Feb 6 2019, 5:38 AM
bu5hm4n updated this revision to Diff 19211.Feb 6 2019, 5:38 AM

update a to An in Eina_Value, so we have two places with correct grammer, so we can laugh at the other cases waiting in this header file :)

bu5hm4n updated this revision to Diff 19212.Feb 6 2019, 5:46 AM

git is a hard tool to use ... sometimes

bu5hm4n updated this revision to Diff 19213.Feb 6 2019, 6:08 AM

passing test

Closed by commit rEFL0f32bb904767: eo: here comes reflection API (authored by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyFeb 7 2019, 5:44 AM
This revision was automatically updated to reflect the committed changes.