Page MenuHomePhabricator

eo: Improve documentation of internal _efl_add_internal_start_external_constructor
Needs RevisionPublic

Authored by felipealmeida on Jul 17 2019, 6:27 PM.



The function is renamed form _efl_add_internal_start_bindings. The function is meant to be used when bindings need to customize constructor call, for example to pass data when a class defined in the bound language gets instantiating from C.

Test Plan

meson test

Diff Detail

rEFL core/efl
No Linters Available
No Unit Test Coverage
Build Status
Buildable 12304
Build 8940: arc lint + arc unit
felipealmeida created this revision.Jul 17 2019, 6:27 PM
felipealmeida requested review of this revision.Jul 17 2019, 6:27 PM

Test looks fine, i will wait on @segfaultxavi for checking out the docs.


Can you add some sort of remark that this callback *needs* to call efl_constructor.

felipealmeida added inline comments.Jul 17 2019, 11:20 PM

That's already mentioned in the external_ctor param

bu5hm4n accepted this revision.Jul 17 2019, 11:25 PM
bu5hm4n added inline comments.

Thank you :) Will Wait now for Xavi.

This revision is now accepted and ready to land.Jul 17 2019, 11:25 PM
segfaultxavi requested changes to this revision.Jul 18 2019, 4:44 AM

Well, I am barely following what is going on here, put the name _efl_add_internal_start_external_constructor is hideous...
Do we really need to have both external and internal in the same name?
This method adds two new parameters to _efl_add_internal_start: external_ctor and sub_ctor_data, obviously they should match.
And here's maybe the solution: let's call it sub_ctor or secondary_ctor, use this name in both parameters, and rename the method to something like _efl_add_internal_start_with_sub_constructor.

Also, I have absolutely no idea what am I allowed to do in the secondary constructor. Is there any method I must call? efl_constructor() maybe? In what order?
I know this method is meant to be internal but unless we document things clearly our technical debt just grows and grows.

This revision now requires changes to proceed.Jul 18 2019, 4:44 AM