This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Merge _LIBCPP_FUNC_VIS, _LIBCPP_TYPE_VIS and _LIBCPP_EXCEPTION_ABI into _LIBCPP_EXPORTED_FROM_ABI
ClosedPublic

Authored by philnik on Jun 11 2023, 11:27 AM.

Details

Summary

These macros are always defined identically, so we can simplify the code a bit by merging them.

Diff Detail

Event Timeline

philnik created this revision.Jun 11 2023, 11:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2023, 11:27 AM
philnik requested review of this revision.Jun 11 2023, 11:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2023, 11:27 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Jun 12 2023, 3:48 PM

This makes sense to me, however we also probably want to take a look at the other _VIS macros and see if we can remove them in favor of _LIBCPP_EXPORTED_FROM_ABI or variants of it (e.g. applying type_visibility(default) at the namespace scope to get rid of _LIBCPP_ENUM_VIS).

Heads up @smeenai, does this make sense to you?

libcxx/include/__algorithm/shuffle.h
24

Commit message typo: identially

25

Missing .clang-format updates and VisibilityMacros.rst changes?

This revision is now accepted and ready to land.Jun 12 2023, 3:48 PM
philnik edited the summary of this revision. (Show Details)Jun 14 2023, 10:16 AM

Yeah, I feel like there might have been some reason for the separate macros, but I can't remember anymore, and a git search didn't find anything relevant. Consolidating them makes sense to me.

How come you're not removing the _LIBCPP_EXCEPTION_ABI definition in __config as well?

https://github.com/llvm/llvm-project/blob/main/libcxx/docs/DesignDocs/VisibilityMacros.rst should be updated as part of this change.

philnik updated this revision to Diff 531423.Jun 14 2023, 10:41 AM
philnik marked 2 inline comments as done.

Address comments

EricWF added a subscriber: EricWF.Aug 16 2023, 9:54 PM

For the record, the need for different macros comes from Windows which, in the past, has needed to distinguish between types, functions, exported things, and imported things when using dllimport/export semantics. Especially during the DLL build.

But if we don't need to support that, good riddance.

libcxx/include/__config
535

This dead code show why the multiple macros were needed. Though it seems like this code has been broken since these large refactoring started.

They're also more important during the DLL build than when using the DLL. When you're using the DLL, they all get defined the same. When you're building the DLL, things are a lot trickier.

We should be clear that we've dropped support for a DLL build on Windows.