Message ID | 20240724195050.4006283-1-jwakely@redhat.com |
---|---|
State | New |
Headers | show |
Series | [1/2] libstdc++: Move std::optional assertions out of _M_get() | expand |
On Wed, 24 Jul 2024 at 22:51, Jonathan Wakely <jwakely@redhat.com> wrote: > > Tested x86_64-linux. > > Any reason not to do this? I don't think the assertions are useful to > catch implementation bugs where we access the contained value without > checking it - we should use tests for that. Looks good to me. > The current assertions also result in repeated invalid bug reports, such > as PR 91281, PR 101659, PR 102712, and PR 107894. I'm not sure moving the assertions helps with that, maybe some of those bug reports are caused by people not knowing how to enable the assertions.
On Wed, 24 Jul 2024 at 20:55, Ville Voutilainen <ville.voutilainen@gmail.com> wrote: > > On Wed, 24 Jul 2024 at 22:51, Jonathan Wakely <jwakely@redhat.com> wrote: > > > > Tested x86_64-linux. > > > > Any reason not to do this? I don't think the assertions are useful to > > catch implementation bugs where we access the contained value without > > checking it - we should use tests for that. > > Looks good to me. Thanks. > > The current assertions also result in repeated invalid bug reports, such > > as PR 91281, PR 101659, PR 102712, and PR 107894. > > I'm not sure moving the assertions helps with that, maybe some of > those bug reports > are caused by people not knowing how to enable the assertions. Oddly, I think *all* of them were people inspecting the code and deciding there were no assertions (because they looked in the wrong place). In some of those bug reports, _GLIBCXX_DEBUG and _GLIBCXX_ASSERTIONS are explicitly mentioned, but they either only looked at the code and didn't test it, or thought they were testing with assertions enabled but failed to enable them somehow.
On Wed, 24 Jul 2024 at 20:58, Jonathan Wakely <jwakely@redhat.com> wrote: > > On Wed, 24 Jul 2024 at 20:55, Ville Voutilainen > <ville.voutilainen@gmail.com> wrote: > > > > On Wed, 24 Jul 2024 at 22:51, Jonathan Wakely <jwakely@redhat.com> wrote: > > > > > > Tested x86_64-linux. > > > > > > Any reason not to do this? I don't think the assertions are useful to > > > catch implementation bugs where we access the contained value without > > > checking it - we should use tests for that. > > > > Looks good to me. > > Thanks. > > > > The current assertions also result in repeated invalid bug reports, such > > > as PR 91281, PR 101659, PR 102712, and PR 107894. > > > > I'm not sure moving the assertions helps with that, maybe some of > > those bug reports > > are caused by people not knowing how to enable the assertions. > > Oddly, I think *all* of them were people inspecting the code and > deciding there were no assertions (because they looked in the wrong > place). In some of those bug reports, _GLIBCXX_DEBUG and > _GLIBCXX_ASSERTIONS are explicitly mentioned, but they either only > looked at the code and didn't test it, or thought they were testing > with assertions enabled but failed to enable them somehow. In one case, the same person who had added the assertions claimed there weren't any ;-)
diff --git a/libstdc++-v3/include/std/optional b/libstdc++-v3/include/std/optional index 48e0f3d36f2..af72004645e 100644 --- a/libstdc++-v3/include/std/optional +++ b/libstdc++-v3/include/std/optional @@ -472,17 +472,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION // The _M_get operations have _M_engaged as a precondition. constexpr _Tp& _M_get() noexcept - { - __glibcxx_assert(this->_M_is_engaged()); - return static_cast<_Dp*>(this)->_M_payload._M_get(); - } + { return static_cast<_Dp*>(this)->_M_payload._M_get(); } constexpr const _Tp& _M_get() const noexcept - { - __glibcxx_assert(this->_M_is_engaged()); - return static_cast<const _Dp*>(this)->_M_payload._M_get(); - } + { return static_cast<const _Dp*>(this)->_M_payload._M_get(); } }; /** @@ -958,27 +952,45 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION // Observers. constexpr const _Tp* operator->() const noexcept - { return std::__addressof(this->_M_get()); } + { + __glibcxx_assert(this->_M_is_engaged()); + return std::__addressof(this->_M_get()); + } constexpr _Tp* operator->() noexcept - { return std::__addressof(this->_M_get()); } + { + __glibcxx_assert(this->_M_is_engaged()); + return std::__addressof(this->_M_get()); + } constexpr const _Tp& operator*() const& noexcept - { return this->_M_get(); } + { + __glibcxx_assert(this->_M_is_engaged()); + return this->_M_get(); + } constexpr _Tp& operator*()& noexcept - { return this->_M_get(); } + { + __glibcxx_assert(this->_M_is_engaged()); + return this->_M_get(); + } constexpr _Tp&& operator*()&& noexcept - { return std::move(this->_M_get()); } + { + __glibcxx_assert(this->_M_is_engaged()); + return std::move(this->_M_get()); + } constexpr const _Tp&& operator*() const&& noexcept - { return std::move(this->_M_get()); } + { + __glibcxx_assert(this->_M_is_engaged()); + return std::move(this->_M_get()); + } constexpr explicit operator bool() const noexcept { return this->_M_is_engaged(); }