This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS
ClosedPublic

Authored by smeenai on Nov 21 2016, 2:00 PM.

Details

Summary

It's useful to be able to disable visibility annotations entirely; for
example, if we're building libc++ static to include in another library,
and we don't want any libc++ functions getting exported out of that
library. This is a generalization of _LIBCPP_DISABLE_DLL_IMPORT_EXPORT.

Diff Detail

Repository
rL LLVM

Event Timeline

smeenai updated this revision to Diff 78778.Nov 21 2016, 2:00 PM
smeenai retitled this revision from to [libc++] Add _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS.
smeenai updated this object.
smeenai added reviewers: mclow.lists, EricWF.
smeenai added a subscriber: cfe-commits.
smeenai updated this revision to Diff 78785.Nov 21 2016, 2:24 PM
smeenai edited edge metadata.

Adding documentation per @EricWF's comment

EricWF edited edge metadata.Nov 23 2016, 1:42 AM

There are two usages of _LIBCPP_DISABLE_DLL_IMPORT_EXPORT in CMakeLists.txt and __config_site.in that need to be changed as well.

Also what about users of the existing name _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS? Should we #error to inform them of the change or should we automatically translate it to the new name for them?

There are two usages of _LIBCPP_DISABLE_DLL_IMPORT_EXPORT in CMakeLists.txt and __config_site.in that need to be changed as well.

Oops, will fix.

Also what about users of the existing name _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS? Should we #error to inform them of the change or should we automatically translate it to the new name for them?

I'd prefer #error; that way we can eventually just drop the macro. Ideally there wouldn't be too many users to begin with, since it was only introduced two months ago, and it hasn't made its way to a release yet.

I'll upload a new diff with those changes later today.

smeenai updated this revision to Diff 79358.Nov 27 2016, 12:30 PM
smeenai edited edge metadata.

Addressing comments

smeenai added inline comments.Nov 27 2016, 12:32 PM
include/__config
513–516 ↗(On Diff #79358)

I'd like to remove this before the 4.0 release, if that's all right with you. I don't think there would be many users of the existing macro, and I'd prefer it to not make its way to a release.

smeenai updated this revision to Diff 79359.Nov 27 2016, 12:36 PM

Not changing non-Windows behavior, to make sure I'm not breaking anyone

EricWF accepted this revision.Dec 5 2016, 12:41 AM
EricWF edited edge metadata.

I'd prefer #error; that way we can eventually just drop the macro. Ideally there wouldn't be too many users to begin with, since it was only introduced two months ago, and it hasn't made its way to a release yet.

If it hasn't even made it into a release then we probably don't need the #error at all. Feel free to commit with or w/o it.

Either way this LGTM.

This revision is now accepted and ready to land.Dec 5 2016, 12:41 AM
This revision was automatically updated to reflect the committed changes.