Page MenuHomePhabricator

Inconsistencies in the usage of `Eina_Hash` make it unsuitable for bindings.
Open, NormalPublic

Description

The main problem here is that Eina_Hash is not flexible enough to support the many ways it can be declared in eolian, in a way that it is still possible to keep track of pointer keys and free them.
I looked at the usage of Eina_Hash in many places in C code and, as it seems, it always works under the assumption that keys can just be removed, and the free function data_free_cb will take care of releasing any data that it is holding. That seems to be true even when deleting the entire hash with eina_hash_free.
This assumption makes sense and looks similar to the way hashes are used in many programming languages, like C#, Python, C++, etc.

But in Eina_Hash, the data_free_cb does not supply a pointer to the key, only to the value.
C code works around that by using a struct as the value whenever it wants to use a key that needs to be released; the key is stored in the struct so it can be freed in the free callback.
This condition imposes some limitations, especially in eolian and the language bindings. Notations like:

hash<stringshare, ...>
hash<Efl.Object, ...>

can not be used if the hash is expected to be the one owning the key it is storing (i.e. not having to keep the key alive somewhere else).

Supplying a pointer to the key in the free callback would be the simplest solution, but it breaks API.
Forcing the value to always be a struct in such cases and the binding code having to translate that under the hood would also be a possibility, but this would need to be agreed upon. Also, things like field order and different free callbacks for each key-value pair would have to be defined.

Also:
Another complication is having to pass a pointer to the *actual* pointer when dealing with hashes created with eina_hash_pointer_new. This one can be worked around but is still an inconvenience.
Bindings like C# have to constantly allocate memory just to store the pointer value being passed.
This problem would be easily avoided if the pointer hash did as the eina_hash_stringshared_new do, taking advantage that the key is always stored as a pointer and just checking the pointer value (and using a key_lenght of zero).
This has the benefits of:

  • Making pointer hashes easier to use, since local variables would no longer be needed in order to pass the pointer value.
  • Using less memory (8 bytes less for each entry).
  • Making eina_hash_add and eina_hash_direct_add consistently the same, since the allocation size would always be zero.
vitor.sousa triaged this task as Normal priority.
cedric added a comment.Aug 7 2019, 9:29 AM

I think the easier solution is to forbid hash on eolian API. It follow our current strategy of not exposing internal type and instead favor iterator and accessor which are a bit more future proof.

cedric added a comment.Aug 7 2019, 9:31 AM

(Change of behavior on eina API is a no go as it is a released API and won't be rewritten for efl unified API)

bu5hm4n added a project: Restricted Project.Aug 20 2019, 11:55 PM