[libcxx-commits] [PATCH] D152378: [libc++][filesystem] Use _LIBCPP_HIDE_FROM_ABI in common headers

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 3 inline comments as done.
ldionne added a comment.

In D152378#4404281 <https://reviews.llvm.org/D152378#4404281>, @philnik wrote:

> How would users end up with multiple versions of a function? All the functions are exclusively in `libc++.a`, and linking against multiple versions or that will definitely blow up.

Why would linking against multiple versions of `libc++.a` blow up? I know it's not a good idea but I'm curious if you know of specific concerns, except e.g. global stream initialization.

In D152378#4406337 <https://reviews.llvm.org/D152378#4406337>, @Mordante wrote:

> I actually prefer consistency in our code base so using _Uglified everywhere. It also makes it easier to move code from the dylib to the header. (Obviously that has its own issues.)

That's my preference as well.



================
Comment at: libcxx/src/filesystem/filesystem_common.h:119
 template <>
-_LIBCPP_CONSTEXPR_SINCE_CXX14 void error_value<void>() {}
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void error_value<void>() {}
 template <>
----------------
Mordante wrote:
> It would be nice to do this in a followup. The dylib requires C++20.
Made a TODO locally.


================
Comment at: libcxx/src/filesystem/filesystem_common.h:121
 template <>
-bool error_value<bool>() {
+inline _LIBCPP_HIDE_FROM_ABI bool error_value<bool>() {
   return false;
----------------
Mordante wrote:
> IIRC inline is not needed for templates, or am I mistaken?
This is an explicit specialization, I think those are not linkonce-odr by default. Godbolt confirms this: https://godbolt.org/z/Wzb4Esb35


================
Comment at: libcxx/src/filesystem/posix_compat.h:219
+inline _LIBCPP_HIDE_FROM_ABI
 int stat_file(const wchar_t *path, StatT *buf, DWORD flags) {
   WinHandle h(path, FILE_READ_ATTRIBUTES, flags);
----------------
Mordante wrote:
> This `wchar_t` is not guarded by `_LIBCPP_HAS_NO_WIDE_CHARACTERS`, maybe something for a followup patch.
Added a todo locally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152378