[libc++] Work around dynamic linking of stringbuf::str() on Windows

[pfusik]

Summary

https://github.com/llvm/llvm-project/issues/40363 caused the C++20
str() const & and str() && to be dllimport'ed despite _LIBCPP_HIDE_FROM_ABI.
This is a temporary solution until #40363 is fixed.

[hans]

libcxx/include/sstream

359

Moving this one seems unrelated?

[pfusik]

libcxx/include/sstream

359

It is related. This overload does not conflict with the pre-C++20 one, so it shouldn't depend on _LIBCPP_BUILDING_LIBRARY.

[hans]

libcxx/include/sstream

359

I mean it doesn't seem related to the problem we're working around with _LIBCPP_HIDE_FROM_ABI_SSTREAM, so perhaps it would be better to do it in a separate patch?

[pfusik]

libcxx/include/sstream

359

It's the same problem: overload missing with _LIBCPP_BUILDING_LIBRARY, just a different solution.
I don't have a strong opinion on whether this should be one patch or two.

[hans]

libcxx/include/sstream

359

I think because it's a template, this one does not have the linking issues that the others do.

I also don't feel strongly about this change, just want to make sure I understand it.

[pfusik]

libcxx/include/sstream

359

Oh, you are right!

[hans]

I wasn't sure if you were planning to update the patch to not move the str(const _SAlloc& __sa) const template, but either way is fine by me really.

[pfusik]

I think it's better as-is.

[Mordante]

libcxx/include/sstream

232

This is our typical pattern.

232

Note this feels a bit of a hack, however since it's temporary I've no objection.

233

Please add a this link in the commit message too. We've changed bug trackers before so having an explicit link makes it easier to find in the future.

341

Why move this hunk?

[pfusik]

libcxx/include/sstream

232

It was suggested by @ldionne in https://reviews.llvm.org/D153709 as a short-term solution.
BTW. can I link to inline comments in Differential?

341

It was an error to have this overload conditional on _LIBCPP_BUILDING_LIBRARY in the first place.
It shouldn't depend on _LIBCPP_BUILDING_LIBRARY because it doesn't conflict with the pre-C++20 str() const overload.
In this patch I change decorations of members under this condition, but this template method isn't problematic at all.

I put it here originally just to follow the order in the spec.

[pfusik]

Fix TODO pattern.
Add full issue link in the commit message.

[pfusik]

libcxx/include/sstream

233

Why don't I see the updated commit message here? I used arc diff.

[pfusik]

Update the commit message with arc diff --edit.

[pfusik]

libcxx/include/sstream

233

Nevermind, I found https://secure.phabricator.com/T2386 :)

[h-vetinari]

libcxx/include/sstream

233

So clang>=17 doesn't have this bug anymore? I ask because https://github.com/llvm/llvm-project/issues/40363 is still open.

[pfusik]

libcxx/include/sstream

233

The plan is to fix this bug before the clang 17 release.

[Mordante]

Thanks LGTM. I leave the final approval to @ldionne.

libcxx/include/sstream

232

Not sure you can link to them, you can when clicking on the link in the generic overview.

I found Louis' suggestion without a hypen.

233

Nevermind, I found https://secure.phabricator.com/T2386 :)

Typically I just edit the commit message on Phab after I submitted a review. This will be applied to the patch when landing.

341

Thanks for the information.

[ldionne]

LGTM. Note that this might be fixed by D155713 in Clang, but this patch is still needed in the meantime.

libcxx/include/sstream

 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 _LIBCPP_PUSH_MACROS
 #include <__undef_macros>
 
 
+// TODO(LLVM 19): Remove this once we drop support for Clang 16,
+// which had this bug: https://github.com/llvm/llvm-project/issues/40363
+#ifdef _WIN32
+#define _LIBCPP_HIDE_FROM_ABI_SSTREAM _LIBCPP_ALWAYS_INLINE
+#else
+#define _LIBCPP_HIDE_FROM_ABI_SSTREAM _LIBCPP_HIDE_FROM_ABI
+#endif
+
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 // Class template basic_stringbuf [stringbuf]
 
 template <class _CharT, class _Traits, class _Allocator>
 class _LIBCPP_TEMPLATE_VIS basic_stringbuf
     : public basic_streambuf<_CharT, _Traits>
 {

 
 #if _LIBCPP_STD_VER >= 20
     _LIBCPP_HIDE_FROM_ABI allocator_type get_allocator() const noexcept { return __str_.get_allocator(); }
 #endif
 
 #if _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_BUILDING_LIBRARY)
     string_type str() const;
 #else
-    _LIBCPP_HIDE_FROM_ABI string_type str() const & { return str(__str_.get_allocator()); }
+    _LIBCPP_HIDE_FROM_ABI_SSTREAM string_type str() const & { return str(__str_.get_allocator()); }
 
-    template <class _SAlloc>
-      requires __is_allocator<_SAlloc>::value
-    _LIBCPP_HIDE_FROM_ABI basic_string<char_type, traits_type, _SAlloc> str(const _SAlloc& __sa) const {
-        return basic_string<_CharT, _Traits, _SAlloc>(view(), __sa);
-    }
-
-    _LIBCPP_HIDE_FROM_ABI string_type str() && {
+    _LIBCPP_HIDE_FROM_ABI_SSTREAM string_type str() && {
         const basic_string_view<_CharT, _Traits> __view = view();
         string_type __result(std::move(__str_), __view.data() - __str_.data(), __view.size());
         __str_.clear();
         __init_buf_ptrs();
         return __result;
     }
 #endif // _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_BUILDING_LIBRARY)
 
 #if _LIBCPP_STD_VER >= 20
+    template <class _SAlloc>
+      requires __is_allocator<_SAlloc>::value
+    _LIBCPP_HIDE_FROM_ABI basic_string<char_type, traits_type, _SAlloc> str(const _SAlloc& __sa) const {
+        return basic_string<_CharT, _Traits, _SAlloc>(view(), __sa);
+    }
+
     _LIBCPP_HIDE_FROM_ABI basic_string_view<char_type, traits_type> view() const noexcept;
 #endif
 
     void str(const string_type& __s) {
         __str_ = __s;
         __init_buf_ptrs();
     }