Page MenuHomePhabricator

evil: Rename EAPI macro to EVIL_API in Evil library
ClosedPublic

Authored by felipealmeida on Oct 19 2020, 8:17 PM.

Details

Summary

Patch from a series of patches to rename EAPI symbols to specific
library DSOs.

EAPI was designed to be able to pass

__attribute__ ((visibility ("default")))``` for symbols with
GCC, which would mean that even if -fvisibility=hidden was used
when compiling the library, the needed symbols would get exported.

MSVC __almost__ works like GCC (or mingw) in which you can
declare everything as export and it will just work (slower, but
it will work). But there's a caveat: global variables will not
work the same way for MSVC, but works for mingw and GCC.

For global variables (as opposed to functions), MSVC requires
correct DSO visibility for MSVC: instead of declaring a symbol as
export for everything, you need to declare it as import when
importing from another DSO and export when defining it locally.

With current EAPI definitions, we get the following example
working in mingw and MSVC (observe it doesn't define any global
variables as exported symbols).

Example 1:
dll1:

EAPI void foo(void);

EAPI void bar()
{
  foo();
}

dll2:

EAPI void foo()
{
  printf ("foo\n");
}

This works fine with API defined as __declspec(dllexport) in both
cases and for gcc defining as

__atttribute__((visibility("default")))

However, the following:
Example 2:

dll1:

EAPI extern int foo;
EAPI void foobar(void);

EAPI void bar()
{
  foo = 5;
  foobar();
}

dll2:

EAPI int foo = 0;
EAPI void foobar()
{
  printf ("foo %d\n", foo);
}

This will work on mingw but will not work for MSVC. And that's why
EAPI is the only solution that worked for MSVC.

Co-authored-by: João Paulo Taylor Ienczak Zanette <jpaulotiz@gmail.com>
Co-authored-by: Ricardo Campos <ricardo.campos@expertise.dev>
Co-authored-by: Lucas Cavalcante de Sousa <lucks.sousa@gmail.com>

Diff Detail

Repository
rEFL core/efl
Branch
arcpatch-D12182
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17388
Build 11650: arc lint + arc unit
felipealmeida created this revision.Oct 19 2020, 8:17 PM
felipealmeida requested review of this revision.Oct 19 2020, 8:17 PM

do you think that, if evil_api.h and eina_api.h are the same, we could write a macro efl_api(LIB) and we replace LIB with EVIL, EINA etc... ?

src/lib/evil/evil_api.h
19 ↗(On Diff #31221)

#elif instead (less indentation)

28 ↗(On Diff #31221)

which case could it be ?

do you think that, if evil_api.h and eina_api.h are the same, we could write a macro efl_api(LIB) and we replace LIB with EVIL, EINA etc... ?

Sorry, I didn't get it what you mean. Do you mean EFL_API_EINA EFL_API_EVIL or, EFL_API(EINA) macro invocation returns true?

src/lib/evil/evil_api.h
19 ↗(On Diff #31221)

That would make it compilable just for Windows or GCC, wouldn't it? What if someone uses a different compiler (a proprietary one for example).

28 ↗(On Diff #31221)

Some proprietary compiler?

something like

#define EFLAPI(EFLLIB) \
#ifdef _WIN32 \

ifndef EFLLIB_STATIC \

etc...

src/lib/evil/evil_api.h
19 ↗(On Diff #31221)

no

#elif __GNUC__
# if __GNUC__ >= 4
etc...
#else
28 ↗(On Diff #31221)

thinking of this, suncc maybe

i think that icc is defining GNUC, I don't know about others like acc

something like

#define EFLAPI(EFLLIB) \
#ifdef _WIN32 \

  1. ifndef EFLLIB_STATIC \ etc...

That's not a valid syntax AFAIK. You can't have preprocessor directives in macro defitinions.

felipealmeida added inline comments.Oct 20 2020, 7:44 AM
src/lib/evil/evil_api.h
28 ↗(On Diff #31221)

There a lot of compilers based on Watcom and other frontends. I used ARM proprietary compiler some years back, they didn't define __GNUC__.

Now I know what you mean with elif btw, just if/elif/else in the same indentation. I can change that.

Change EVIL_API if/else preprocessor directives to have less indentation

Everything seems fine, but just to be sure, is there a real need to include evil_api.h at evil_stdlib.h and evil_dlfcn.h as it is already included at evil_private.h?

For me, it seems that including it only on evil_private.h would fit EFL's private header inclusion standard better.

Everything seems fine, but just to be sure, is there a real need to include evil_api.h at evil_stdlib.h and evil_dlfcn.h as it is already included at evil_private.h?

For me, it seems that including it only on evil_private.h would fit EFL's private header inclusion standard better.

It is better that when you use something, you include it. So modifications in other parts of the headers won't break something else. Just like you may get away with using size_t without include'ing stddef.h, but it is a good practice to include it anwyay.

ProhtMeyhet edited the summary of this revision. (Show Details)Oct 23 2020, 4:36 PM
ProhtMeyhet added a subscriber: ProhtMeyhet.

I took the liberty to correct your code quotation in your post. Please watch out in the future for this.

Everything seems fine, but just to be sure, is there a real need to include evil_api.h at evil_stdlib.h and evil_dlfcn.h as it is already included at evil_private.h?

For me, it seems that including it only on evil_private.h would fit EFL's private header inclusion standard better.

It is better that when you use something, you include it. So modifications in other parts of the headers won't break something else. Just like you may get away with using size_t without include'ing stddef.h, but it is a good practice to include it anwyay.

then include always evil_api.h in all files, not just some files.

the usage is to include evil_private.h when evil is needed, so i think it is not very useful to include evil_api.h in a couple of files, and even having evil_api.h is useless, imho. It gives really no benefit

do you mean you plan to include only evil_stdio.h for example ?

felipealmeida added a comment.EditedOct 26 2020, 9:42 AM

Everything seems fine, but just to be sure, is there a real need to include evil_api.h at evil_stdlib.h and evil_dlfcn.h as it is already included at evil_private.h?

For me, it seems that including it only on evil_private.h would fit EFL's private header inclusion standard better.

It is better that when you use something, you include it. So modifications in other parts of the headers won't break something else. Just like you may get away with using size_t without include'ing stddef.h, but it is a good practice to include it anwyay.

then include always evil_api.h in all files, not just some files.

the usage is to include evil_private.h when evil is needed, so i think it is not very useful to include evil_api.h in a couple of files, and even having evil_api.h is useless, imho. It gives really no benefit

do you mean you plan to include only evil_stdio.h for example ?

Do you think I should just add EVIL_API to evil_private.h then and have all files include it instead of evil_api.h? I can do that. Let me know if you think that's the best way.

do you mean you plan to include only evil_stdio.h for example ?

Do you think I should just add EVIL_API to evil_private.h then and have all files include it instead of evil_api.h? I can do that. Let me know if you think that's the best way.

for me it's the most simple : if a code is added to a source file, no need to add a evil header file if needed.

Removed evil_api.h

Removed #undef setlocale modification and made that another commit. This depends on D12184

Fix evil_locale.c not include'ing evil_locale.h

vtorri added inline comments.Oct 26 2020, 11:26 PM
src/lib/evil/evil_dlfcn.h
5

normally, not needed (as we include evil_private.h)

src/lib/evil/evil_stdlib.h
4

normally, not needed

src/lib/evil/evil_unistd.h
4

normally not needed

felipealmeida added inline comments.Oct 27 2020, 10:11 AM
src/lib/evil/evil_dlfcn.h
5

Don't you think this a bit too much nitpicking?

vtorri accepted this revision.Nov 5 2020, 5:56 AM
This revision is now accepted and ready to land.Nov 5 2020, 5:56 AM
jptiz accepted this revision.Nov 10 2020, 5:25 AM
This revision was automatically updated to reflect the committed changes.