[libcxx-commits] [PATCH] D103960: [libc++] Towards a simpler extern template story in libc++

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/docs/ReleaseNotes.rst:69-72
+- The ``_LIBCPP_DISABLE_EXTERN_TEMPLATE`` is not honored anymore when defined by users
+  of libc++. Instead, users not wishing to take a dependency on libc++ should link
+  against the static version of libc++, which will result in no dependency being
+  taken against the shared library.
----------------
@ldionne wrote:
> does have the problem that if we want to be able to enable the debugging mode, the library has to have been built with the debug mode enabled, which wasn't the case previously

It was always the case for debug iterators. I wrote an email to the libcxx-dev mailing list on 2021-05-01 and still haven't seen any reply to it.


================
Comment at: libcxx/include/__locale:343
+extern template class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS collate<char>;
+extern template class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS collate<wchar_t>;
 
----------------
This PR basically LGTM. I do want to raise the question of greppability: The old macros let you `grep -l _LIBCPP_EXTERN_TEMPLATE` to find these sneaky declarations. The new code makes you `grep -l extern.template` instead. This //seems// fine to me; do you agree?

FYI, some places already use `extern template` (for function templates only):
```
libcxx/include/locale:extern template _LIBCPP_FUNC_VIS time_base::dateorder __time_get_storage<_CharT>::__do_date_order() const; \
libcxx/include/locale:extern template _LIBCPP_FUNC_VIS __time_get_storage<_CharT>::__time_get_storage(const char*); \
libcxx/include/locale:extern template _LIBCPP_FUNC_VIS __time_get_storage<_CharT>::__time_get_storage(const string&); \
libcxx/include/locale:extern template _LIBCPP_FUNC_VIS void __time_get_storage<_CharT>::init(const ctype<_CharT>&); \
libcxx/include/locale:extern template _LIBCPP_FUNC_VIS __time_get_storage<_CharT>::string_type __time_get_storage<_CharT>::__analyze(char, const ctype<_CharT>&); \
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103960/new/

https://reviews.llvm.org/D103960