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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision as: Mordante.
Mordante added a comment.

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

> In D152378#4403678 <https://reviews.llvm.org/D152378#4403678>, @ldionne wrote:
>
>> In D152378#4403648 <https://reviews.llvm.org/D152378#4403648>, @philnik wrote:
>>
>>> I understand that, but why not simply remove the anonymous namespace? What's the point of additionally adding `_LIBCPP_HIDE_FROM_ABI`?
>>
>> I guess we build the dylib with `-fvisibility=hidden`, so we don't need it technically. That was the original purpose. We could also argue that it's more consistent with the rest of the code, but I agree it's technically not needed.
>>
>> So I can either keep it for consistency, or just remove the `namespace { }` bit. Thoughts?
>
> Most of the stuff in `src/` is written very similar to the headers, but IMO we should strive to make that stuff more maintainable. i.e. avoid anything that isn't required, like _Uglification, adding `_LIBCPP_HIDE_FROM_ABI` everywhere etc. I'm pretty sure everybody would love to remove that shit in the headers if it weren't necessary.

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.)

LGTM!



================
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 <>
----------------
It would be nice to do this in a followup. The dylib requires C++20.


================
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;
----------------
IIRC inline is not needed for templates, or am I mistaken?


================
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);
----------------
This `wchar_t` is not guarded by `_LIBCPP_HAS_NO_WIDE_CHARACTERS`, maybe something for a followup patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152378