[libcxx-commits] [PATCH] D155185: [libc++] Work around dynamic linking of stringbuf::str() on Windows

[Mark de Wever via Phabricator via libcxx-commits]

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

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



================
Comment at: libcxx/include/sstream:232
 
+// TODO(LLVM 19): Remove this once we drop support for Clang 16,
+// which had this bug: https://github.com/llvm/llvm-project/issues/40363
----------------
pfusik wrote:
> Mordante wrote:
> > Mordante wrote:
> > > This is our typical pattern.
> > Note this feels a bit of a hack, however since it's temporary I've no objection.
> 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?
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.


================
Comment at: libcxx/include/sstream:233
+// 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
----------------
pfusik wrote:
> pfusik wrote:
> > Mordante wrote:
> > > 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.
> > Why don't I see the updated commit message here? I used `arc diff`.
> Nevermind, I found https://secure.phabricator.com/T2386 :)
> 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.


================
Comment at: libcxx/include/sstream:341
-      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);
----------------
pfusik wrote:
> Mordante wrote:
> > Why move this hunk?
> 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.
Thanks for the information.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155185