Details
- Reviewers
ldionne
Diff Detail
- Repository
- rCXX libc++
Event Timeline
Answers based on my understanding of how the availability attributes work.
include/future | ||
---|---|---|
531 | I don't think it is necessary to mark non API facing types with the availability attribute. So this one does not look right. Also, we currently export the member functions of __assoc_sub_state from the dylib. Removing the _LIBCPP_TYPE_VIS means that those would not be exported anymore if we started building with -fvisibility=hidden, so removing the visibility attribute does not look right. | |
2301 | Not an API-facing type. | |
2372 | Look OK. | |
2446 | Looks OK. | |
include/shared_mutex | ||
310 | We don't seem to be exporting anything related to shared_lock from the dylib, so I would leave this out. |
include/shared_mutex | ||
---|---|---|
310 | The dylib doesn't have any symbols related to deque, either, right? Yet libc++ does class _LIBCPP_TEMPLATE_VIS deque I guess I still want the simple rule to be "if it's part of the library's public API _or_ mangled into the dylib, it gets a visibility attribute." |
Re-add _LIBCPP_TYPE_VIS to __assoc_sub_state because it is mentioned in the mangled names of functions exported from src/future.cpp.
Remove _LIBCPP_AVAILABILITY_FUTURE from types that aren't supposed to be visible to user-programmers anyway (IIUC).
include/shared_mutex | ||
---|---|---|
310 | For non-template types, no visibility attribute should mean it is hidden. For templates, default visibility could be used to allow users to explicitly instantiate templates in their dylibs and have those be visible. However, we should not support that as they could also apply an attribute to the explicit instantiation, which is cleaner. In the future, we want no visibility annotations at all except on the few things we export from the dylib. And hence this one would not have an attribute. |
Discussed offline with @ldionne and my verdict was "If there is any rule for adding these attributes, then the current codebase doesn't follow that rule very well." So, abandoning; and if there's any ad-hoc fix to be made in the areas touched by this patch, then Louis can make it.
I don't think it is necessary to mark non API facing types with the availability attribute. So this one does not look right.
Also, we currently export the member functions of __assoc_sub_state from the dylib. Removing the _LIBCPP_TYPE_VIS means that those would not be exported anymore if we started building with -fvisibility=hidden, so removing the visibility attribute does not look right.