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

Wed Jun 1 01:07:36 PDT 2022

philnik added inline comments.

================
Comment at: libcxx/include/ios:698
 template <class _CharT, class _Traits>
-basic_ios<_CharT, _Traits>::~basic_ios()
+inline basic_ios<_CharT, _Traits>::~basic_ios()
 {
----------------
jloser wrote:
> jloser wrote:
> > ayzhao wrote:
> > > I'm not sure if I should specify `_LIBCPP_INLINE_VISIBILITY` here - does anyone have any ideas?
> > Should we use `inline _LIBCPP_INLINE_VISIBILITY` instead like the neighboring code currently does?
> So, `_LIBCPP_INLINE_VISIBILITY` is a historical predecessor to `_LIBCPP_HIDE_FROM_ABI`.  In newer code, we've been updating to `_LIBCPP_HIDE_FROM_ABI`.
> 
> With that being said, we definitely want `inline` there and also either `_LIBCPP_HIDE_FROM_ABI` or `_LIBCPP_INLINE_VISIBILITY`.  There's an argument to be made for both. Using `_LIBCPP_HIDE_FROM_ABI` is the "new de-facto standard", whereas the latter is locally consistent in this neighborhood of code. I don't feel strongly.
> 
> It's a bit surprising this destructor wasn't hidden from being an exported symbol in the ABI!
I think nobody would have a problem if we just remove the out-of-line definition and change line 642 to `_LIBCPP_HIDE_FROM_ABI virtual ~basic_ios() = default;`. I //think// it should be fine to make it `= default`, because all copy- and move constructors are deleted.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126738