Skip to content
This repository has been archived by the owner on Mar 26, 2020. It is now read-only.

Add support for -fvisibility=hidden and -fvisibility=default flags #415

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

csabaszigetipix4d
Copy link

I quote the updated README.md here :

Support for -fvisibility=hidden and -fvisibility=default

Djinni supports both the -fvisibility=hidden and the -fvisibility=default flags.

The symbols that are belonging to the public interfaces that are generated by Djinni are automatically defined to be visible symbols.
To achieve this Djinni is marking the generated code with PROJECT_EXPORT.
PROJECT_EXPORT is defined in support-lib/djinni_common.hpp.
This macro is using the correct attribute specifier for the actual compiler. For more details, please check the macro definition in the support-lib/djinni_common.hpp header.
This PROJECT_EXPORT does nothing in case if you are using the -fvisibility=default compiler flag.
This PROJECT_EXPORT in case if you are using the -fvisibility=default compiler flag.

Since the generated headers are including this support-lib/djinni_common.hpp, you must distribute it together with your library that is using Djinni.

Windows

For Windows builds, you must define BUILDING_DLL based on your needs.
You can define it as a compiler option like -DBUILDING_DLL.
For more details, please check the macro definition in the support-lib/djinni_common.hpp header.

…ject_export.hpp

1.: Added new Djinni support-lib header : project_export.hpp
2.: Moved PROJECT_EXPORT definition from djinni_common.hpp to the new project_export.hpp file
3.: Remove every " // needed for PROJECT_EXPORT" comments
4.: Updated README
5.: Fixed automated tests of Djinni
@xianwen
Copy link

xianwen commented Dec 27, 2018

Hi, @csabaszigetipix4d:
I noticed you haven't signed the CLA yet, could you please sign it here: https://opensource.dropbox.com/cla/
Thanks a lot!

Copy link
Contributor

@artwyman artwyman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only taken a high-level look so far. I think the idea of making visibility controllable is a good one, but it should probably be configurable rather than having a single default behavior. As I mention in one of my inline comments, I don't think that exporting these specific symbols is always what's right for all use cases.

@@ -6,6 +6,7 @@ include(GNUInstallDirs)

set(SRC_SHARED
"support-lib/djinni_common.hpp"
"support-lib/project_export.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used so widely (requiring it to be in the include path) that it really needs a more unique name, like djinni_project_export.hpp.


You can pass `-fvisibility=hidden` or the `-fvisibility=default` flags to your compiler, Djinni handles both cases well.

The symbols that are belonging to the public interfaces that are generated by Djinni are automatically defined to be visible symbols.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like it's not obvious that making these symbols always public is the right behavior for all users. E.g. if Djinni symbols are only used internally to a larger library, they wouldn't need to be exported. In the use case we've used at Dropbox, we build all our C++ code into a single library on Android, so only the JNI functions need to be exported.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants