-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
[libc++] Remove workaround which allows setting _LIBCPP_OVERRIDABLE_FUNC_VIS externally #113139
Conversation
…UNC_VIS externally
@llvm/pr-subscribers-libcxx Author: Nikolas Klauser (philnik777) Changes
Full diff: https://github.com/llvm/llvm-project/pull/113139.diff 1 Files Affected:
diff --git a/libcxx/include/__config b/libcxx/include/__config
index fcba56f7e3d5b1..4155fd33dabc44 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -408,11 +408,7 @@ typedef __char32_t char32_t;
# define _LIBCPP_EXPORTED_FROM_ABI _LIBCPP_VISIBILITY("default")
# define _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS _LIBCPP_VISIBILITY("default")
# define _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS
-
-// TODO: Make this a proper customization point or remove the option to override it.
-# ifndef _LIBCPP_OVERRIDABLE_FUNC_VIS
-# define _LIBCPP_OVERRIDABLE_FUNC_VIS _LIBCPP_VISIBILITY("default")
-# endif
+# define _LIBCPP_OVERRIDABLE_FUNC_VIS _LIBCPP_VISIBILITY("default")
# if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
// The inline should be removed once PR32114 is resolved
|
CC @rnk |
CC @llvm/libcxx-vendors @philnik777 Can you please add a short explanation of the history of this flag? I think you removed it and that broke a use case, and now it can be removed. What broke when you originally removed it? |
Some people used a custom new/delete, which requires them to have default visibility, but didn't want to leak any symbols otherwise. Since the new/delete visibility can now be controlled by a compiler flag, there is no reason to set it with out macro. |
I think Louis is asking for more context links, which would be helpful for reviewers and future readers. My best efforts to find the history turned up https://reviews.llvm.org/D129768, but I couldn't find any more useful history on the Chromium side. It looks like Chromium no longer users this macro, but I can't figure out if it ever did or when that changed. |
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.
LGTM, but please make sure to include the explanation for the history of this change in the PR description (i.e. the commit message once landed).
-fvisibility-global-new-delete
has been added in Clang 18, so there is no more need to specify the visibility of new/delete via libc++-internal macros.Some people used a custom new/delete, which requires them to have default visibility, but didn't want to leak any symbols otherwise. Since the new/delete visibility can now be controlled by a compiler flag, there is no reason to set it with out macro.
https://reviews.llvm.org/D128007 originally tried to remove the option to override the macro, but was partially reverted because people actually set this specific macro.