| Message ID | alpine.DEB.2.02.1705140854260.4227@stedding.saclay.inria.fr |
|---|---|
| State | New |
| Headers | show |
On 14/05/17 09:19 +0200, Marc Glisse wrote: >Hello, > >this patch adds 2 simple __glibcxx_assert in optional that match the >precondition in the comment above. I am not sure if there was a reason >the author wrote that comment instead of the assertion, but constexpr >use still seems to work. Yes, in a constexpr context we get the following when it's not engaged: In file included from /home/jwakely/gcc/8/include/c++/8.0.0/utility:68:0, from /home/jwakely/gcc/8/include/c++/8.0.0/optional:36, from opt.cc:1: /home/jwakely/gcc/8/include/c++/8.0.0/optional: In function ‘int main()’: opt.cc:11:22: in constexpr expansion of ‘f()’ opt.cc:6:11: in constexpr expansion of ‘o.std::optional<int>::operator*()’ /home/jwakely/gcc/8/include/c++/8.0.0/optional:708:29: in constexpr expansion of ‘((std::optional<int>*)this)->std::optional<int>::<anonymous>.std::_Optional_base<int>::_M_get()’ /home/jwakely/gcc/8/include/c++/8.0.0/optional:390:2: error: call to non-constexpr function ‘void std::__replacement_assert(const char*, int, const char*, const char*)’ __glibcxx_assert(_M_is_engaged()); ^ I think that's an improvement over what we have now: opt.cc: In function ‘int main()’: opt.cc:11:22: in constexpr expansion of ‘f()’ opt.cc:11:23: error: accessing ‘std::_Optional_payload<int, true, true>::<unnamed union>::_M_payload’ member instead of initialized ‘std::_Optional_payload<int, true, true>::<unnamed union>::_M_empty’ member in constant expression constexpr int i = f(); ^ So the patch is OK for trunk, thanks.
Index: include/std/optional =================================================================== --- include/std/optional (revision 248008) +++ include/std/optional (working copy) @@ -379,25 +379,31 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } // The following functionality is also needed by optional, hence the // protected accessibility. protected: constexpr bool _M_is_engaged() const noexcept { return this->_M_payload._M_engaged; } // The _M_get operations have _M_engaged as a precondition. constexpr _Tp& _M_get() noexcept - { return this->_M_payload._M_payload; } + { + __glibcxx_assert(_M_is_engaged()); + return this->_M_payload._M_payload; + } constexpr const _Tp& _M_get() const noexcept - { return this->_M_payload._M_payload; } + { + __glibcxx_assert(_M_is_engaged()); + return this->_M_payload._M_payload; + } // The _M_construct operation has !_M_engaged as a precondition // while _M_destruct has _M_engaged as a precondition. template<typename... _Args> void _M_construct(_Args&&... __args) noexcept(is_nothrow_constructible<_Stored_type, _Args...>()) { ::new (std::__addressof(this->_M_payload._M_payload)) _Stored_type(std::forward<_Args>(__args)...); Index: testsuite/20_util/optional/cons/value_neg.cc =================================================================== --- testsuite/20_util/optional/cons/value_neg.cc (revision 248008) +++ testsuite/20_util/optional/cons/value_neg.cc (working copy) @@ -30,15 +30,15 @@ int main() struct X { explicit X(int) {} }; std::optional<X> ox{42}; std::optional<X> ox2 = 42; // { dg-error "conversion" } std::optional<std::unique_ptr<int>> oup{new int}; std::optional<std::unique_ptr<int>> oup2 = new int; // { dg-error "conversion" } struct U { explicit U(std::in_place_t); }; std::optional<U> ou(std::in_place); // { dg-error "no matching" } - // { dg-error "no type" "" { target { *-*-* } } 487 } - // { dg-error "no type" "" { target { *-*-* } } 497 } - // { dg-error "no type" "" { target { *-*-* } } 554 } + // { dg-error "no type" "" { target { *-*-* } } 493 } + // { dg-error "no type" "" { target { *-*-* } } 503 } + // { dg-error "no type" "" { target { *-*-* } } 560 } } }