Page MenuHomePhabricator

fix eolian function redefine errors
Closed, ResolvedPublic

Description

export EOLIAN_WARN_FUNC_DUPLICATES=1

and then build EFL.

Task is to fix those 100 warnings gracefully.

We won't fix legacy code warnings. e.g. elm_interface_scrollable.eo

jpeg added a comment.Dec 18 2017, 9:20 PM

We won't fix legacy code warnings. e.g. elm_interface_scrollable.eo

Actually it would be nice to fix them OR find a way to hide the warnings. I want the end result to be zero warning when compiling EFL. :)

jpeg added a comment.EditedJan 18 2018, 11:28 PM

Current status:

eolian: efl_gfx_path.eo:259:7: function 'interpolate' redefined (originally at efl_vg.eo:96:7)
eolian: efl_gfx_path.eo:62:7: function 'bounds_get' redefined (originally at ector_renderer.eo:110:7)
eolian: efl_gfx_path.eo:62:7: function 'bounds_get' redefined (originally at efl_vg.eo:85:7)
eolian: efl_net_control_access_point.eo:161:13: function 'name' redefined (originally at efl_object.eo:63:10)
eolian: efl_net_control_technology.eo:70:13: function 'name' redefined (originally at efl_object.eo:63:10)
eolian: efl_net_session.eo:89:13: function 'name' redefined (originally at efl_object.eo:63:10)
eolian: efl_player.eo:44:14: function 'position' redefined (originally at efl_gfx.eo:21:10)
eolian: efl_text_cursor.eo:37:10: function 'cursor' redefined (originally at efl_ui_cursor.eo:43:10)
eolian: efl_text_font.eo:59:17: function 'font' redefined (originally at efl_text_properties.eo:21:10)
eolian: efl_text_font.eo:66:17: function 'font_source' redefined (originally at efl_text_properties.eo:48:10)
eolian: efl_ui_focus_object.eo:29:13: function 'focus' redefined (originally at elm_widget_item.eo:154:17)
eolian: efl_ui_list_model.eo:32:10: function 'size' redefined (originally at efl_gfx.eo:39:10)
eolian: efl_ui_model_factory_connect.eo:5:7: function 'connect' redefined (originally at efl_ui_model_connect.eo:5:7)
eolian: efl_ui_text.eo:146:10: function 'password' redefined (originally at efl_text_format.eo:102:17)
eolian: efl_ui_zoom.eo:53:10: function 'zoom' redefined (originally at efl_gfx_map.eo:253:7)
eolian: efl_vg.eo:17:10: function 'name' redefined (originally at efl_object.eo:63:10)

efl_gfx_path, efl_vg -> @smohanty
efl_net -> @barbieri
efl_player -> @taxi2se
efl_ui_list, efl_ui_model_factory_connect -> @felipealmeida
efl_ui_text -> @herdsman
efl_ui_zoom -> @singh.amitesh

jpeg added a subscriber: taxi2se.Jan 19 2018, 12:41 AM

interesting, what's the expectation about name?

The base object provides that, mostly for debug purposes... almost unused except in some examples we want to print out later.

However, many specific items, particularly models (person-name?) or similar (as for efl.net.control.access_point: the name of the AP), will include that.

IF models and AP had to use another property for that, it would be weird. If they used the base object, it would be confusing.

Hint: in Python the base stuff is exposed with some prefixes, such as __ to avoid conflicting with user-defined properties, like __str__ method to get a string representation, __dir__ to list fields of the object, etc... In GrapghQL, it's the same, __typename and so on...

jpeg added a comment.Jan 22 2018, 4:17 AM

Good question. In the base object class we get efl_name_get() for C.
So far in case of a method name clash like this we would just "prefix" the conflicting APIs. Example here Efl.Ui.Win.win_name gives obj.win_name() in bindings, efl_ui_win_name() in C. But the window name is not very useful, while the AP (EEID maybe?) and technology names are very useful.
Not sure what to do... call them "ap_name" or "eeid" and "technology" or "technology_name"? Hmm...
While I find that __ prefix is not a bad idea, it should also apply to all other methods in Efl.Object to be consistent. eolian_gen would need to know about it (so that base functions like efl_del remain as-is, for instance).

why not demote efl_name, since it's mostly for debug/tracking purposes.

Ex, in Python every class has a name, which is not exposed in cls.name, rather as cls.__name__, since as said it's very common to have a Person.name, etc... same for class documentation: cls.__doc__, etc...

I don't like the idea of the underscore prefix as suggested by @barbieri: in python the obj.__xxx__ notation i used with a really clear definition: all of them are special methods that are automatically called by the language in specific case, fe the obj.__str__method is called when you do print(obj), the obj.__init__ is called when you create an instance, the obj.__gt__ is called when you do obj1 > obj2... an so on. A really clear and explicit definition, while our eo.name is not special in any way, nor it's different from efl.net.tech.name. I also cannot find a rule/definition to underscore eo.name... at least without underscoring all Efl.Object methods (_parent, _comment, _del, etc).

@barbieri also say that eo.name is "almost unused". This is not true, at least in my code. I use name a lot in all my apps for various purposes (not debug). This is probably the case because python is OOP and dynamically typed, thus using class names is much more powerfull than in C.

I can see 2 working solutions:

  1. rename efl.net to something more specific (eeid, tech_name, etc) as already pointed out by @jpeg
  2. rename eo.name to eo.obj_name

I prefer solution #1 for a simple reason:
eo.name is used/present in ALL of our classes (thus I expect a widely usage) while eeid and tech_name will be used only in rare applications that need to manage your network connectivity.

zmike removed a subscriber: zmike.Jan 22 2018, 10:55 AM
jpeg added a comment.Jan 22 2018, 9:58 PM

So far simple renames have been the preferred method, even if it sometimes leads to less than perfect method names.

Maybe we should find a way to deal in languages that do not support calling base functions where a derived function with the same name exists. Generators could prefix names with the base class name for repeated functions.

For C++ and C# we can fully qualify the name if we want so we can call unambiguously any method. For JS and Lua, OTH, that is not possible, so a workaround would be necessary.

I'm starting to feel that we can't be toally succesful trying to avoid name clashes.

jpeg added a comment.Jan 29 2018, 9:47 PM

Meh. We are down to ~10 APIs or less that clash in the Unified API. It seems easier to me to use a tiny bit of creativity here rather than add complexity to all our binding generators (or API users, in case of C++). Note that in C++ a name clash can be worked around, but a newly introduced name clash will prevent existing apps from compiling (as obj.my_func now resolves to two real functions, C++ compiler will fail to figure out which one).
I think it's more important to continue resolving the clashes, then somehow discard any error/warning for legacy-only EO files, but straight out error in case of a Unified API EO file.

@zmike did that get fixed by your patchs actually ?

@jpeg, @barbieri, this task is very related to T6847, right?

I don't see the redefinition warnings you mention anymore (using EOLIAN_WARN_FUNC_DUPLICATES=1). Does this mean the clashes have been fixed already?

zmike added a comment.Apr 2 2018, 2:07 PM

Everything shown with EOLIAN_WARN_FUNC_DUPLICATES=1 should have been fixed as of a month or two ago.

cedric closed this task as Resolved.Apr 2 2018, 2:21 PM

Great !

singh.amitesh added a commit: Restricted Diffusion Commit.Aug 20 2018, 1:01 PM