This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Use _LIBCPP_TYPE_VIS_ONLY with enum class
ClosedPublic

Authored by smeenai on Aug 30 2016, 5:09 PM.

Details

Summary

An enum class has associated type info. In the Microsoft ABI, type info
is emitted in the COMDAT section and isn't exported, so clang rightfully
complains about __declspec(dllexport) being unused for an enum class.
On other platforms, we still want to export the type info.

Diff Detail

Repository
rL LLVM

Event Timeline

smeenai updated this revision to Diff 69786.Aug 30 2016, 5:09 PM
smeenai retitled this revision from to [libc++] Don't attempt to dllexport enum class.
smeenai updated this object.
smeenai added reviewers: abdulras, EricWF, mclow.lists.
smeenai added subscribers: cfe-commits, kastiglione.
compnerd edited reviewers, added: compnerd; removed: abdulras.Aug 30 2016, 9:20 PM
compnerd added inline comments.Aug 30 2016, 9:29 PM
include/__config
719 ↗(On Diff #69786)

I don't think that this is right. On non-Windows, this would potentially expand out to __attribute__ (( __type_visibility__ ("default") )). I don't believe that enum classes have anything like RTTI associated with them, so we don't really want type visibility for that.

smeenai added inline comments.Aug 30 2016, 9:37 PM
include/__config
719 ↗(On Diff #69786)

Good point. Should I just drop the visibility macro entirely then?

compnerd added inline comments.Aug 30 2016, 9:54 PM
include/__config
719 ↗(On Diff #69786)

I think that might be the correct approach here. The definition will be inlined in the header, so the values should always be available, so the visibility attributes shouldn't change anything anyways.

smeenai updated this revision to Diff 69797.Aug 30 2016, 10:04 PM
smeenai updated this object.

Removing export entirely, per compnerd's suggestion

smeenai marked 3 inline comments as done.Aug 30 2016, 10:05 PM
compnerd edited edge metadata.Sep 1 2016, 2:28 PM

Ugh, I was wrong. There is type information associated with a scoped enum. This means that _LIBCPP_TYPE_VIS is the right annotation here.

smeenai updated this revision to Diff 70087.Sep 1 2016, 3:54 PM
smeenai retitled this revision from [libc++] Don't attempt to dllexport enum class to [libc++] Use _LIBCPP_TYPE_VIS_ONLY with enum class.
smeenai updated this object.
smeenai edited edge metadata.

Switching back to _LIBCPP_TYPE_VIS_ONLY after discussion with @compnerd

Friendly ping :)

EricWF accepted this revision.Sep 9 2016, 1:56 PM
EricWF edited edge metadata.
This revision is now accepted and ready to land.Sep 9 2016, 1:56 PM
This revision was automatically updated to reflect the committed changes.