Page MenuHomePhabricator

Eina: more macros and Types
Needs ReviewPublic

Authored by ali.alzyod on Thu, Jun 20, 5:19 AM.

Details

Summary

To simplify development on EFL:

1- Having special symbol for internal APIs can simplify detecting these internal APIs (where they are export internally between EFL parts)

Diff Detail

Repository
rEFL core/efl
Branch
arcpatch-D9140
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12082
Build 8873: arc lint + arc unit
ali.alzyod created this revision.Thu, Jun 20, 5:19 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/

ali.alzyod requested review of this revision.Thu, Jun 20, 5:19 AM

I am updating part of text-block and instead of define byte internally we can use it from Eina.

Also for internal function there are no good indicator if it is internal or not (we need to search) , so this might help

ali.alzyod retitled this revision from Eina: more macros to Eina: more macros and Types.
ali.alzyod edited the summary of this revision. (Show Details)Thu, Jun 20, 5:26 AM

should EAPI_INTERNAL EAPI really be part of the API ?

should EAPI_INTERNAL EAPI really be part of the API ?

The Issue is that there are many internal APIs like the ones in evas_common, and there are no good indicator to detect these APIs
(updating internal api will not break user code if we change them, so may be it is just help us detect these apis)

bu5hm4n requested changes to this revision.Tue, Jul 9, 1:41 AM

I have my problem with this revision because of:

This revision now requires changes to proceed.Tue, Jul 9, 1:41 AM
ali.alzyod added a comment.EditedTue, Jul 9, 3:47 AM

@bu5hm4n

Even if something is flagged as EAPI_INTERNAL everyone can still use it, there is no mechanism protecting the user from just using the API

This is right I am not targeting API user, this flag used to let API developer/maintainer to know that this is internal API.

Why do we need Eina_Byte ? you can just use stdint.h and get a much widget accepted and much wider defined range of data types.

Mainly two reasons :
1- It is easy to read, Byte vs (uint_8_t,unsigned char)

for example if function fill color components in RGB24  is  (Byte R, Byte G, Byte B) which is easy to read than (uint8_t R, uint8_t G, uint8_t B)

2- I think it is logical in library to use its own types (same as with Eina_Bool)

 So in all places you will find Eina_Byte for things the expect byte,  currently we use int in many places for values that should be bytes.
 
 For example look at:
 https://www.enlightenment.org/develop/api/ref/c/key/efl_gfx_color_get
We use int instead of (unsigned char   or uint_8),  but if we have Eina_Byte (or Eina_uByte)  these problems will be reduced
zmike requested changes to this revision.Tue, Jul 9, 6:00 AM

@bu5hm4n

Even if something is flagged as EAPI_INTERNAL everyone can still use it, there is no mechanism protecting the user from just using the API

This is right I am not targeting API user, this flag used to let API developer/maintainer to know that this is internal API.

Why do we need Eina_Byte ? you can just use stdint.h and get a much widget accepted and much wider defined range of data types.

Mainly two reasons :
1- It is easy to read, Byte vs (uint_8_t,unsigned char)

for example if function fill color components in RGB24  is  (Byte R, Byte G, Byte B) which is easy to read than (uint8_t R, uint8_t G, uint8_t B)

It's more difficult to read for developers new to EFL than uint8_t, which we should have been using all along but were not due to portability concerns. These concerns no longer exist for stdint types.

2- I think it is logical in library to use its own types (same as with Eina_Bool)

 So in all places you will find Eina_Byte for things the expect byte,  currently we use int in many places for values that should be bytes.
 
 For example look at:
 https://www.enlightenment.org/develop/api/ref/c/key/efl_gfx_color_get
We use int instead of (unsigned char   or uint_8),  but if we have Eina_Byte (or Eina_uByte)  these problems will be reduced

I'm not at all in favor of adding more typedefs which aren't strictly necessary. uint8_t is immediately understandable to everyone. We should use this.

ali.alzyod updated this revision to Diff 23161.Tue, Jul 9, 9:29 AM

remove defining new datatype

ali.alzyod edited the summary of this revision. (Show Details)Tue, Jul 9, 9:31 AM
ali.alzyod added a comment.EditedTue, Jul 9, 9:51 AM

@zmike @bu5hm4n

For EAPI_INTERNAL

This patch targeting APIs developer/maintainer (not users) to know that this is internal API.

For example
(1) I know if I change the signature of internal API (flagged as EAPI_INTERNAL), then I will not create compitablity issue for EFL developers (there code will combile normally)
(2) EFL commiters/reviewers will not mistaken internal APIs with public API, and it can help preventing cases break public API by mistake.

bu5hm4n requested changes to this revision.Wed, Jul 10, 7:00 AM

I appreciate your idea here. However:

  1. history has shown now multiple times, everything that is in a public header will be used, noone checks if this has EAPI_INTERNAL infront of it or not.
  2. The efl way of doing that is to create a header like "eo_internal.h" or "evas_private.h" which contains the API which is meant for internal usage, those private header files are then also not exposed or installed. Which means, a user would need to explictly copy the definition, by the time he does so, its his responsibility of maintaining it, so not our problem

Given those two facts i prefer to not merge this symbol, but rather continue how we did it for now.

This revision now requires changes to proceed.Wed, Jul 10, 7:00 AM
ali.alzyod added a comment.EditedWed, Jul 10, 7:15 AM

@bu5hm4n
I think there are misunderstanding about the target of this patch.
I am not targeting API user at all, I am targeting us, or anyone writing apis for EFL. (I will request review again because I feel I did not clear enough what the patch target)

history has shown now multiple times, everything that is in a public header will be used, noone checks if this has EAPI_INTERNAL infront of it or not.

May be my english is not good :), Again this is not targetting API users, I do not care about API user, if he want to use internal API it is his own problem.

The efl way of doing that is to create a header like "eo_internal.h" or "evas_private.h" which contains the API which is meant for internal usage, those private header files are then also not exposed or installed. Which means, a user would need to explictly copy the definition, by the time he does so, its his responsibility of maintaining it, so not our problem

Same as before, I am not targeting API user, I am targeting API writer.

This the main problem, internal api are written all over places, and there are no easy way to check if it is internal API or not (you need to start looking for evidence ).

For example, how can you know that API function in evas_font.h are internal APIs ? you need to look around and search where is the header and who (other modules inside EFL).

ali.alzyod requested review of this revision.Wed, Jul 10, 11:48 AM