From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2181) id CDD2A3857C45; Tue, 6 Jun 2023 22:58:35 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org CDD2A3857C45 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1686092315; bh=0rUJTQmdBZXEqZNYdSyxcW99+eGIs1/NqPzqaVqzRS0=; h=From:To:Subject:Date:From; b=pzp5A2DUcQ2YAyOz9PaWXNc/D553Tt6/bkAJDGvERCs8kDNxl3wcIWn8sdQMkS0Lo GH97zmM77zT3Uy9kG2bQY8aZjgOUHhjdc7AMsme5T6VbaJZLXxVZqBJkPGmsqwXC4x /JgxJA2wi0AhHkYP/2zHyWo2GRRfx0I/+i5xefms= MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="utf-8" From: Jonathan Wakely To: gcc-cvs@gcc.gnu.org, libstdc++-cvs@gcc.gnu.org Subject: [gcc r13-7423] libstdc++: Do not use std::expected::value() in monadic ops (LWG 3938) X-Act-Checkin: gcc X-Git-Author: Jonathan Wakely X-Git-Refname: refs/heads/releases/gcc-13 X-Git-Oldrev: b14121a2ac6dc36dcd3053f8a672ec3bb7ab6695 X-Git-Newrev: ff58310f274e496c6ce6ef86211be734d94e5dee Message-Id: <20230606225835.CDD2A3857C45@sourceware.org> Date: Tue, 6 Jun 2023 22:58:35 +0000 (GMT) List-Id: https://gcc.gnu.org/g:ff58310f274e496c6ce6ef86211be734d94e5dee commit r13-7423-gff58310f274e496c6ce6ef86211be734d94e5dee Author: Jonathan Wakely Date: Thu Jun 1 11:16:49 2023 +0100 libstdc++: Do not use std::expected::value() in monadic ops (LWG 3938) The monadic operations in std::expected always check has_value() so we can avoid the execptional path in value() and the assertions in error() by accessing _M_val and _M_unex directly. This means that the monadic operations no longer require _M_unex to be copyable so that it can be thrown from value(), as modified by LWG 3938. This also fixes two incorrect uses of std::move in transform(F&&)& and transform(F&&) const& which I found while making these changes. Now that move-only error types are supported, it's possible to properly test the constraints that LWG 3877 added to and_then and transform. The lwg3877.cc test now does that. libstdc++-v3/ChangeLog: * include/std/expected (expected::and_then, expected::or_else) (expected::transform_error): Use _M_val and _M_unex instead of calling value() and error(), as per LWG 3938. (expected::transform): Likewise. Remove incorrect std::move calls from lvalue overloads. (expected::and_then, expected::or_else) (expected::transform): Use _M_unex instead of calling error(). * testsuite/20_util/expected/lwg3877.cc: Add checks for and_then and transform, and for std::expected. * testsuite/20_util/expected/lwg3938.cc: New test. (cherry picked from commit fe94f8b7e022b7e154f6c47cc292d4463bddac5e) Diff: --- libstdc++-v3/include/std/expected | 78 ++++++----- libstdc++-v3/testsuite/20_util/expected/lwg3877.cc | 145 +++++++++++++++++---- libstdc++-v3/testsuite/20_util/expected/lwg3938.cc | 142 ++++++++++++++++++++ 3 files changed, 298 insertions(+), 67 deletions(-) diff --git a/libstdc++-v3/include/std/expected b/libstdc++-v3/include/std/expected index 5ea0d6a7cb9..a63557448f7 100644 --- a/libstdc++-v3/include/std/expected +++ b/libstdc++-v3/include/std/expected @@ -745,8 +745,7 @@ namespace __expected { if (_M_has_value) [[likely]] return std::move(_M_val); - _GLIBCXX_THROW_OR_ABORT(bad_expected_access<_Er>( - std::move(_M_unex))); + _GLIBCXX_THROW_OR_ABORT(bad_expected_access<_Er>(std::move(_M_unex))); } constexpr _Tp&& @@ -754,8 +753,7 @@ namespace __expected { if (_M_has_value) [[likely]] return std::move(_M_val); - _GLIBCXX_THROW_OR_ABORT(bad_expected_access<_Er>( - std::move(_M_unex))); + _GLIBCXX_THROW_OR_ABORT(bad_expected_access<_Er>(std::move(_M_unex))); } constexpr const _Er& @@ -849,9 +847,9 @@ namespace __expected static_assert(is_same_v); if (has_value()) - return std::__invoke(std::forward<_Fn>(__f), value()); + return std::__invoke(std::forward<_Fn>(__f), _M_val); else - return _Up(unexpect, error()); + return _Up(unexpect, _M_unex); } template requires is_constructible_v<_Er, const _Er&> @@ -863,9 +861,9 @@ namespace __expected static_assert(is_same_v); if (has_value()) - return std::__invoke(std::forward<_Fn>(__f), value()); + return std::__invoke(std::forward<_Fn>(__f), _M_val); else - return _Up(unexpect, error()); + return _Up(unexpect, _M_unex); } template requires is_constructible_v<_Er, _Er> @@ -877,9 +875,9 @@ namespace __expected static_assert(is_same_v); if (has_value()) - return std::__invoke(std::forward<_Fn>(__f), std::move(value())); + return std::__invoke(std::forward<_Fn>(__f), std::move(_M_val)); else - return _Up(unexpect, std::move(error())); + return _Up(unexpect, std::move(_M_unex)); } @@ -892,9 +890,9 @@ namespace __expected static_assert(is_same_v); if (has_value()) - return std::__invoke(std::forward<_Fn>(__f), std::move(value())); + return std::__invoke(std::forward<_Fn>(__f), std::move(_M_val)); else - return _Up(unexpect, std::move(error())); + return _Up(unexpect, std::move(_M_unex)); } template requires is_constructible_v<_Tp, _Tp&> @@ -906,9 +904,9 @@ namespace __expected static_assert(is_same_v); if (has_value()) - return _Gr(in_place, value()); + return _Gr(in_place, _M_val); else - return std::__invoke(std::forward<_Fn>(__f), error()); + return std::__invoke(std::forward<_Fn>(__f), _M_unex); } template requires is_constructible_v<_Tp, const _Tp&> @@ -920,9 +918,9 @@ namespace __expected static_assert(is_same_v); if (has_value()) - return _Gr(in_place, value()); + return _Gr(in_place, _M_val); else - return std::__invoke(std::forward<_Fn>(__f), error()); + return std::__invoke(std::forward<_Fn>(__f), _M_unex); } @@ -935,9 +933,9 @@ namespace __expected static_assert(is_same_v); if (has_value()) - return _Gr(in_place, std::move(value())); + return _Gr(in_place, std::move(_M_val)); else - return std::__invoke(std::forward<_Fn>(__f), std::move(error())); + return std::__invoke(std::forward<_Fn>(__f), std::move(_M_unex)); } template requires is_constructible_v<_Tp, const _Tp> @@ -949,9 +947,9 @@ namespace __expected static_assert(is_same_v); if (has_value()) - return _Gr(in_place, std::move(value())); + return _Gr(in_place, std::move(_M_val)); else - return std::__invoke(std::forward<_Fn>(__f), std::move(error())); + return std::__invoke(std::forward<_Fn>(__f), std::move(_M_unex)); } template requires is_constructible_v<_Er, _Er&> @@ -967,7 +965,7 @@ namespace __expected _M_val); }); else - return _Res(unexpect, std::move(error())); + return _Res(unexpect, _M_unex); } template requires is_constructible_v<_Er, const _Er&> @@ -983,7 +981,7 @@ namespace __expected _M_val); }); else - return _Res(unexpect, std::move(error())); + return _Res(unexpect, _M_unex); } template requires is_constructible_v<_Er, _Er> @@ -999,7 +997,7 @@ namespace __expected std::move(_M_val)); }); else - return _Res(unexpect, std::move(error())); + return _Res(unexpect, std::move(_M_unex)); } template requires is_constructible_v<_Er, const _Er> @@ -1015,7 +1013,7 @@ namespace __expected std::move(_M_val)); }); else - return _Res(unexpect, std::move(error())); + return _Res(unexpect, std::move(_M_unex)); } template requires is_constructible_v<_Tp, _Tp&> @@ -1026,7 +1024,7 @@ namespace __expected using _Res = expected<_Tp, _Gr>; if (has_value()) - return _Res(in_place, value()); + return _Res(in_place, _M_val); else return _Res(__unexpect_inv{}, [&]() { return std::__invoke(std::forward<_Fn>(__f), @@ -1042,7 +1040,7 @@ namespace __expected using _Res = expected<_Tp, _Gr>; if (has_value()) - return _Res(in_place, value()); + return _Res(in_place, _M_val); else return _Res(__unexpect_inv{}, [&]() { return std::__invoke(std::forward<_Fn>(__f), @@ -1058,7 +1056,7 @@ namespace __expected using _Res = expected<_Tp, _Gr>; if (has_value()) - return _Res(in_place, std::move(value())); + return _Res(in_place, std::move(_M_val)); else return _Res(__unexpect_inv{}, [&]() { return std::__invoke(std::forward<_Fn>(__f), @@ -1074,7 +1072,7 @@ namespace __expected using _Res = expected<_Tp, _Gr>; if (has_value()) - return _Res(in_place, std::move(value())); + return _Res(in_place, std::move(_M_val)); else return _Res(__unexpect_inv{}, [&]() { return std::__invoke(std::forward<_Fn>(__f), @@ -1530,7 +1528,7 @@ namespace __expected if (has_value()) return std::__invoke(std::forward<_Fn>(__f)); else - return _Up(unexpect, error()); + return _Up(unexpect, _M_unex); } template requires is_constructible_v<_Er, const _Er&> @@ -1544,7 +1542,7 @@ namespace __expected if (has_value()) return std::__invoke(std::forward<_Fn>(__f)); else - return _Up(unexpect, error()); + return _Up(unexpect, _M_unex); } template requires is_constructible_v<_Er, _Er> @@ -1558,7 +1556,7 @@ namespace __expected if (has_value()) return std::__invoke(std::forward<_Fn>(__f)); else - return _Up(unexpect, std::move(error())); + return _Up(unexpect, std::move(_M_unex)); } template requires is_constructible_v<_Er, const _Er> @@ -1572,7 +1570,7 @@ namespace __expected if (has_value()) return std::__invoke(std::forward<_Fn>(__f)); else - return _Up(unexpect, std::move(error())); + return _Up(unexpect, std::move(_M_unex)); } template @@ -1586,7 +1584,7 @@ namespace __expected if (has_value()) return _Gr(); else - return std::__invoke(std::forward<_Fn>(__f), error()); + return std::__invoke(std::forward<_Fn>(__f), _M_unex); } template @@ -1600,7 +1598,7 @@ namespace __expected if (has_value()) return _Gr(); else - return std::__invoke(std::forward<_Fn>(__f), error()); + return std::__invoke(std::forward<_Fn>(__f), _M_unex); } template @@ -1614,7 +1612,7 @@ namespace __expected if (has_value()) return _Gr(); else - return std::__invoke(std::forward<_Fn>(__f), std::move(error())); + return std::__invoke(std::forward<_Fn>(__f), std::move(_M_unex)); } template @@ -1628,7 +1626,7 @@ namespace __expected if (has_value()) return _Gr(); else - return std::__invoke(std::forward<_Fn>(__f), std::move(error())); + return std::__invoke(std::forward<_Fn>(__f), std::move(_M_unex)); } template requires is_constructible_v<_Er, _Er&> @@ -1641,7 +1639,7 @@ namespace __expected if (has_value()) return _Res(__in_place_inv{}, std::forward<_Fn>(__f)); else - return _Res(unexpect, error()); + return _Res(unexpect, _M_unex); } template requires is_constructible_v<_Er, const _Er&> @@ -1654,7 +1652,7 @@ namespace __expected if (has_value()) return _Res(__in_place_inv{}, std::forward<_Fn>(__f)); else - return _Res(unexpect, error()); + return _Res(unexpect, _M_unex); } template requires is_constructible_v<_Er, _Er> @@ -1667,7 +1665,7 @@ namespace __expected if (has_value()) return _Res(__in_place_inv{}, std::forward<_Fn>(__f)); else - return _Res(unexpect, std::move(error())); + return _Res(unexpect, std::move(_M_unex)); } template requires is_constructible_v<_Er, const _Er> @@ -1680,7 +1678,7 @@ namespace __expected if (has_value()) return _Res(__in_place_inv{}, std::forward<_Fn>(__f)); else - return _Res(unexpect, std::move(error())); + return _Res(unexpect, std::move(_M_unex)); } template diff --git a/libstdc++-v3/testsuite/20_util/expected/lwg3877.cc b/libstdc++-v3/testsuite/20_util/expected/lwg3877.cc index 556d8d5dc9e..876275bfdb0 100644 --- a/libstdc++-v3/testsuite/20_util/expected/lwg3877.cc +++ b/libstdc++-v3/testsuite/20_util/expected/lwg3877.cc @@ -1,6 +1,8 @@ // { dg-options "-std=gnu++23" } // { dg-do compile { target c++23 } } +// LWG 3877. Incorrect constraints on const-qualified monadic overloads + #include struct T1 @@ -20,45 +22,134 @@ struct T3 T3(const T3&&) { } }; +template +concept Has_and_then = requires(Exp&& exp, F f) { + std::forward(exp).and_then(f); +}; + +using ExpiT1 = std::expected; +static_assert( Has_and_then ); +static_assert( Has_and_then ); +static_assert( Has_and_then ); +static_assert( Has_and_then ); + +using ExpiT2 = std::expected; +static_assert( !Has_and_then ); +static_assert( !Has_and_then ); +static_assert( !Has_and_then ); +static_assert( !Has_and_then ); + +using ExpiT3 = std::expected; +static_assert( Has_and_then ); +static_assert( !Has_and_then ); +static_assert( Has_and_then ); // uses and_then(F) const && +static_assert( Has_and_then ); + template concept Has_or_else = requires(Exp&& exp, F f) { std::forward(exp).or_else(f); }; -using E1 = std::expected; -static_assert( Has_or_else ); -static_assert( Has_or_else ); -static_assert( Has_or_else ); -static_assert( Has_or_else ); +using ExpT1i = std::expected; +static_assert( Has_or_else ); +static_assert( Has_or_else ); +static_assert( Has_or_else ); +static_assert( Has_or_else ); + +using ExpT2i = std::expected; +static_assert( !Has_or_else ); +static_assert( !Has_or_else ); +static_assert( !Has_or_else ); +static_assert( !Has_or_else ); + +using ExpT3i = std::expected; +static_assert( Has_or_else ); +static_assert( !Has_or_else ); +static_assert( Has_or_else ); // uses or_else(F) const && +static_assert( Has_or_else ); + +template +concept Has_transform = requires(Exp&& exp, F f) { + std::forward(exp).transform(f); +}; + +static_assert( Has_transform ); +static_assert( Has_transform ); +static_assert( Has_transform ); +static_assert( Has_transform ); -using E2 = std::expected; -static_assert( !Has_or_else ); -static_assert( !Has_or_else ); -static_assert( !Has_or_else ); -static_assert( !Has_or_else ); +static_assert( !Has_transform ); +static_assert( !Has_transform ); +static_assert( !Has_transform ); +static_assert( !Has_transform ); -using E3 = std::expected; -static_assert( Has_or_else ); -static_assert( !Has_or_else ); -static_assert( Has_or_else ); // uses or_else(F) const && -static_assert( Has_or_else ); +static_assert( Has_transform ); +static_assert( !Has_transform ); +static_assert( Has_transform ); // uses transform(F) const && +static_assert( Has_transform ); template concept Has_transform_error = requires(Exp&& exp, F f) { std::forward(exp).transform_error(f); }; -static_assert( Has_transform_error ); -static_assert( Has_transform_error ); -static_assert( Has_transform_error ); -static_assert( Has_transform_error ); +static_assert( Has_transform_error ); +static_assert( Has_transform_error ); +static_assert( Has_transform_error ); +static_assert( Has_transform_error ); + +static_assert( !Has_transform_error ); +static_assert( !Has_transform_error ); +static_assert( !Has_transform_error ); +static_assert( !Has_transform_error ); + +static_assert( Has_transform_error ); +static_assert( !Has_transform_error ); +static_assert( Has_transform_error ); // uses transform_error(F) const && +static_assert( Has_transform_error ); + +// std::expected + +using ExpvT1 = std::expected; +static_assert( Has_and_then ); +static_assert( Has_and_then ); +static_assert( Has_and_then ); +static_assert( Has_and_then ); + +using ExpvT2 = std::expected; +static_assert( !Has_and_then ); +static_assert( !Has_and_then ); +static_assert( !Has_and_then ); +static_assert( !Has_and_then ); + +using ExpvT3 = std::expected; +static_assert( Has_and_then ); +static_assert( !Has_and_then ); +static_assert( Has_and_then ); // uses and_then(F) const && +static_assert( Has_and_then ); + +using Expvi = std::expected; +static_assert( Has_or_else ); +static_assert( Has_or_else ); +static_assert( Has_or_else ); +static_assert( Has_or_else ); + +static_assert( Has_transform ); +static_assert( Has_transform ); +static_assert( Has_transform ); +static_assert( Has_transform ); + +static_assert( !Has_transform ); +static_assert( !Has_transform ); +static_assert( !Has_transform ); +static_assert( !Has_transform ); -static_assert( !Has_transform_error ); -static_assert( !Has_transform_error ); -static_assert( !Has_transform_error ); -static_assert( !Has_transform_error ); +static_assert( Has_transform ); +static_assert( !Has_transform ); +static_assert( Has_transform ); // uses transform(F) const && +static_assert( Has_transform ); -static_assert( Has_transform_error ); -static_assert( !Has_transform_error ); -static_assert( Has_transform_error ); // uses transform_error(F) const && -static_assert( Has_transform_error ); +static_assert( Has_transform_error ); +static_assert( Has_transform_error ); +static_assert( Has_transform_error ); +static_assert( Has_transform_error ); diff --git a/libstdc++-v3/testsuite/20_util/expected/lwg3938.cc b/libstdc++-v3/testsuite/20_util/expected/lwg3938.cc new file mode 100644 index 00000000000..c7e3758a902 --- /dev/null +++ b/libstdc++-v3/testsuite/20_util/expected/lwg3938.cc @@ -0,0 +1,142 @@ +// { dg-options "-std=gnu++23" } +// { dg-do compile { target c++23 } } + +// LWG 3938. Cannot use std::expected monadic ops with move-only error_type + +#include +#include + +struct MoveOnly { + constexpr MoveOnly(int i) : i(i) { } + constexpr MoveOnly(MoveOnly&&) = default; + constexpr MoveOnly(const MoveOnly&& mo) : i(mo.i) { } + constexpr bool operator==(const MoveOnly&) const = default; + int i; +}; + +constexpr bool +test_and_then() +{ + auto fun = [](int i) { return std::expected(i); }; + + std::expected good(9); + std::expected e1 = std::move(good).and_then(fun); + VERIFY( e1 == good ); + const auto& gooder = good; + std::expected e2 = std::move(gooder).and_then(fun); + VERIFY( e2 == gooder ); + + std::expected bad(std::unexpect, 99); + std::expected e3 = std::move(bad).and_then(fun); + VERIFY( e3 == bad ); + const auto& badder = bad; + std::expected e4 = std::move(badder).and_then(fun); + VERIFY( e4 == badder ); + + auto vun = [] { return std::expected(1); }; + std::expected vud; + std::expected e5 = std::move(vud).and_then(vun); + VERIFY( *e5 == 1 ); + const auto& vudder = vud; + std::expected e6 = std::move(vudder).and_then(vun); + VERIFY( *e6 == 1 ); + + return true; +} + +static_assert( test_and_then() ); + +constexpr bool +test_or_else() +{ + auto fun = [](const MoveOnly&& mo) { return std::expected(mo.i); }; + + std::expected good(9); + std::expected e1 = std::move(good).or_else(fun); + VERIFY( e1 == good ); + const auto& gooder = good; + std::expected e2 = std::move(gooder).or_else(fun); + VERIFY( e2 == gooder ); + + std::expected bad(std::unexpect, 99); + std::expected e3 = std::move(bad).or_else(fun); + VERIFY( *e3 == 99 ); + const auto& badder = bad; + std::expected e4 = std::move(badder).or_else(fun); + VERIFY( *e4 == 99 ); + + auto vun = [](const MoveOnly&& mo) { return std::expected{}; }; + std::expected vud; + std::expected e5 = std::move(vud).or_else(vun); + VERIFY( e5.has_value() ); + const auto& vudder = vud; + std::expected e6 = std::move(vudder).or_else(vun); + VERIFY( e6.has_value() ); + + return true; +} + +static_assert( test_or_else() ); + +constexpr bool +test_transform() +{ + auto fun = [](int i) { return (long)i; }; + + std::expected good(9); + std::expected e1 = std::move(good).transform(fun); + VERIFY( e1 == good ); + const auto& gooder = good; + std::expected e2 = std::move(gooder).transform(fun); + VERIFY( e2 == gooder ); + + std::expected bad(std::unexpect, 99); + std::expected e3 = std::move(bad).transform(fun); + VERIFY( e3 == bad ); + const auto& badder = bad; + std::expected e4 = std::move(badder).transform(fun); + VERIFY( e4 == badder ); + + auto vun = []() { return 1L; }; + std::expected vud; + std::expected e5 = std::move(vud).transform(vun); + VERIFY( *e5 == 1 ); + const auto& vudder = vud; + std::expected e6 = std::move(vudder).transform(vun); + VERIFY( *e6 == 1 ); + + return true; +} + +static_assert( test_transform() ); + +constexpr bool +test_transform_error() +{ + auto fun = [](const MoveOnly&& mo) { return (long)mo.i; }; + + std::expected good(9); + std::expected e1 = std::move(good).transform_error(fun); + VERIFY( e1 == good ); + const auto& gooder = good; + std::expected e2 = std::move(gooder).transform_error(fun); + VERIFY( e2 == gooder ); + + std::expected bad(std::unexpect, 99); + std::expected e3 = std::move(bad).transform_error(fun); + VERIFY( e3.error() == 99 ); + const auto& badder = bad; + std::expected e4 = std::move(badder).transform_error(fun); + VERIFY( e4.error() == 99 ); + + std::expected vud(std::unexpect, 1); + std::expected e5 = std::move(vud).transform_error(fun); + VERIFY( e5.error() == 1 ); + const auto& vudder = vud; + std::expected e6 = std::move(vudder).transform_error(fun); + VERIFY( e6.error() == 1 ); + + return true; +} + +static_assert( test_transform_error() );