-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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.
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
Fix symbol leak #3627
Fix symbol leak #3627
Conversation
Signed-off-by: Vladislav Shchapov <[email protected]>
Fix looks good to me. I can confirm that, at least for our specific case of GCC 9 and CMake target Current
and this PR does not ;)
I'm still personally curious why this special case for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR.
#if !defined(FMT_HEADER_ONLY) && !defined(_WIN32) && \ | ||
(defined(FMT_LIB_EXPORT) || defined(FMT_SHARED)) | ||
# define FMT_INLINE_API FMT_VISIBILITY("default") | ||
#else | ||
# define FMT_INLINE_API | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should introduce a new macro. Let's update FMT_VISIBILITY
instead since we shouldn't be changing visibility in general when not compiling a shared library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vitaut
FMT_VISIBILITY is used unconditionally:
Lines 1883 to 1885 in a4b7b24
/* Use the hidden visibility as a workaround for a GCC bug (#1973). */ \ | |
/* Use a macro-like name to avoid shadowing warnings. */ \ | |
struct FMT_VISIBILITY("hidden") FMT_COMPILE_STRING : base { \ |
Therefore, we need two macros.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, we can simplify this later.
Merged, thanks! |
Signed-off-by: Vladislav Shchapov <[email protected]>
Fix for issue #3626
@feltech, please test this PR.