This avoids using anonymous namespaces in headers and ensures that
the various helper functions get deduplicated across the TUs
implementing <filesystem>. Otherwise, we'd get a definition of
these helper functions in each TU where they are used, which is
entirely unnecessary.
Details
- Reviewers
philnik Mordante - Group Reviewers
Restricted Project - Commits
- rG21853b964555: [libc++][filesystem] Avoid using anonymous namespaces in support headers
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I don't think there is any need to mark the functions as _LIBCPP_HIDE_FROM_ABI. These are all implementation details of the dylib, which is built with -fvisibility=hidden, so these symbols can never escape the dylib, making this pure noise.
These functions live in headers and they are included in multiple .cpp files that are linked together to create the dylib. Without HIDE_FROM_ABI and with the anonymous namespace, we get duplicate definitions in each .o file created using these headers. Also, the compiler complains about internal-linkage functions being unused in some of the .cpp files.
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.
Actually I just thought about this and if users are linking against static libc++.a, then they could potentially end up with symbols with the same name from different versions of libc++, which would be an ODR violation. In other words, we do need the same precautions in our own built library as we have in our headers due to static archives. So I believe _LIBCPP_HIDE_FROM_ABI is necessary if we drop the anonymous namespace. Do you agree?
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.
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!
libcxx/src/filesystem/filesystem_common.h | ||
---|---|---|
119 ↗ | (On Diff #529411) | It would be nice to do this in a followup. The dylib requires C++20. |
121 ↗ | (On Diff #529411) | IIRC inline is not needed for templates, or am I mistaken? |
libcxx/src/filesystem/posix_compat.h | ||
248 | This wchar_t is not guarded by _LIBCPP_HAS_NO_WIDE_CHARACTERS, maybe something for a followup patch. |
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.
That's my preference as well.
libcxx/src/filesystem/filesystem_common.h | ||
---|---|---|
119 ↗ | (On Diff #529411) | Made a TODO locally. |
121 ↗ | (On Diff #529411) | This is an explicit specialization, I think those are not linkonce-odr by default. Godbolt confirms this: https://godbolt.org/z/Wzb4Esb35 |
libcxx/src/filesystem/posix_compat.h | ||
248 | Added a todo locally. |
libcxx/src/filesystem/filesystem_common.h | ||
---|---|---|
121 ↗ | (On Diff #529411) | Thanks for the info! |
Right, obviously, not sure what I was thinking. This means we could just drop the anonymous namespace instead and it would work as well. I'd prefer using HIDE_FROM_ABI because it makes this clearer and more consistent with the rest of the code base, but let's start with just dropping anonymous namespaces.
This wchar_t is not guarded by _LIBCPP_HAS_NO_WIDE_CHARACTERS, maybe something for a followup patch.