Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't specify visibility attributes for static libraries #2157

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Don't specify visibility attributes for static libraries #2157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions libarchive/archive.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,13 @@ typedef ssize_t la_ssize_t;
# define __LA_DECL __declspec(dllimport)
# endif
# endif
#elif defined __LIBARCHIVE_ENABLE_VISIBILITY
#elif defined __LIBARCHIVE_ENABLE_VISIBILITY && (!defined LIBARCHIVE_STATIC)
# define __LA_DECL __attribute__((visibility("default")))
#else
/* Static libraries or non-Windows needs no special declaration. */
/*
* Static libraries on all platforms and shared libraries on non-Windows
* platforms that don't support visibility annotations.
*/
# define __LA_DECL
Copy link
Contributor

Choose a reason for hiding this comment

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

At a glance a few preexisting issues have contributed to this:

  • internal specifics __LA_DECL and by extension LIBARCHIVE_STATIC and __LIBARCHIVE_BUILD have made their way into the public headers

In one of my projects, I a) defined FOO_API symbol internally for non-win32 gnuc builds (stub elsewhere) and b) annotated the definition (aka c file). The windows (msvc or mingw) builds are using a .spec file listing the exported symbols.

This would be my top recommendation.

  • the existing if/else chain is borderline incomprehensive

If the above suggestion doesn't fly, something like the following should be much better IMHO.

#if defined foo_static
#define foo /* dummy */
#if defined _WIN32 // no need for __WIN32__ or cygwin
#elif defined libarchive_building
#define foo __declspec(dllexport) // no need for gnu variant, gcc 4.0 (and maybe earlier) supports the msvc syntax
#else
#define foo __declspec(dllimport) // ditto no need for gnuc/gcc check
#endif
#elif defined __GNUC__ && __GNUC__ >= 4 // this is the explicit recommendation by the GCC docs, over the manual in-configure checks 
#define foo __attribute...
#else
#define foo /* dummy */
#endif

Hope that helps.

#endif

Expand Down
7 changes: 5 additions & 2 deletions libarchive/archive_entry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,10 +120,13 @@ typedef ssize_t la_ssize_t;
# define __LA_DECL __declspec(dllimport)
# endif
# endif
#elif defined __LIBARCHIVE_ENABLE_VISIBILITY
#elif defined __LIBARCHIVE_ENABLE_VISIBILITY && (!defined LIBARCHIVE_STATIC)
# define __LA_DECL __attribute__((visibility("default")))
#else
/* Static libraries on all platforms and shared libraries on non-Windows. */
/*
* Static libraries on all platforms and shared libraries on non-Windows
* platforms that don't support visibility annotations.
*/
# define __LA_DECL
#endif

Expand Down