On Tue, 3 Dec 2024 at 16:56, Jonathan Wakely <jwak...@redhat.com> wrote:
>
> On Tue, 3 Dec 2024 at 16:19, Giuseppe D'Angelo
> <giuseppe.dang...@kdab.com> wrote:
> >
> > Hello,
> >
> > The attached patch changes std::weak_ptr "converting move
> > constructor/assignment" -- that is, from a rvalue weak_ptr<Derived> to a
> > weak_ptr<Base>.
> >
> > In the general case, such conversion requires lock()ing the weak_ptr
> > because the pointed-to object might have already been destroyed, and a
> > Derived->Base pointer conversion requires dereferencing the pointer iff
> > Base is a virtual base class of Derived. Therefore the correct thing to
> > do in the general case is to lock() the weak_ptr, call get() on the
> > resulting shared_ptr, and do the pointer conversion while this
> > shared_ptr is alive.
> >
> > However, the newly added __is_virtual_base_of builtin allows us to
> > micro-optimize this pointer upcast, and avoid calling lock() (and paying
> > for the associated atomic operations) in case Base is *not* a virtual
> > base class of Derived. In this case we can "just" perform the upcast;
> > as far as I know, no ABI requires dereferencing the pointer in the
> > non-virtual inheritance case.
>
> This is a nice optimization!
>
> We would usually use _S_safe_upcast for the name of a static member
> function, although I think __safe_upcast is OK here.
>
> Do we need the _Compatible constraint on __safe_upcast? It's only
> called internally from code that has already enforced that constraint,
> so checking again just slows compilation down.
>
> I've been moving from using `if _GLIBCXX17_CONSTEXPR` to just
> surrounding the function in diagnostic pragmas so that we can use `if
> constexpr` unconditionally, i.e.
>
> #pragma GCC diagnostic push
> #pragma GCC diagnostic pushignored "-Wc++17-extensions" // if constexpr
> ...
> #pragma GCC diagnostic pop
>
> That way we get the benefits of `if constexpr` even in C++11 mode.
>
> We still have lots of code using _GLIBCXX17_CONSTEXPR but I think we
> should phase it out in favour of using `if constexpr` unconditionally
> (or at least using `if _GLIBCXX_CONSTEXPR` for code which needs to
> compile as C++98 too).
Ah, I think we also need to guard the use of the built-in with a
preprocessor check to ensure it's available. So maybe invert the sense
of the condition, so that the fast path is only present when the
built-in is supported:
#if __has_builtin(__builtin_is_virtual_base_of)
if constexpr (!__builtin_is_virtual_base_of(_Tp, _Yp))
return __r._M_ptr;
else
#endif
return __r.lock().get();
We should probably even be using the _GLIBCXX_USE_BUILTIN_TRAIT macro
here, so that its use can be disabled.
