[libcxx-commits] [PATCH] D126738: [libc++][NFC] Inline the definition of ~basic_ios()

[Alan Zhao via Phabricator via libcxx-commits]

ayzhao added a comment.

In D126738#3550831 <https://reviews.llvm.org/D126738#3550831>, @ldionne wrote:

> In D126738#3550536 <https://reviews.llvm.org/D126738#3550536>, @philnik wrote:
>
>> LGTM assuming CI passes, but please wait for a second #libc <https://reviews.llvm.org/tag/libc/> reviewer before landing the patch.
>
> I'm pretty sure CI is going to fail during `check-cxx-abilist`.
>
> As-is, this patch is an ABI break for any user of `iostream` because it removes `std::__1::basic_ios<wchar_t, std::__1::char_traits<wchar_t> >::~basic_ios()` and `std::__1::basic_ios<char, std::__1::char_traits<char> >::~basic_ios()` from the dylib. Furthermore, I believe there was probably an intent of this destructor acting as a key function so that the vtable and RTTI of `basic_ios<char|wchar_t>` ends up in the dylib, and in the dylib only. I'm not 100% sure about that, but that's the most likely explanation for this strange definition in the first place.
>
> After reading https://crbug.com/1327706#c13, it seems like the issue comes up when enabling libc++'s debug mode, right? If so, then I think this must be tackled via D122941 <https://reviews.llvm.org/D122941> or something like it, instead.

I spoke with cjdb@ offline and it seems that we can maintain ABI compatibility by defining an empty destructor (i.e. `~basic_ios() {}`) instead of using `= default`: https://godbolt.org/z/r73v1oE6n. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126738