From cdf71bdca64536a005f2491d8c19f1d05a1c62f6 Mon Sep 17 00:00:00 2001 From: Herb Sutter Date: Tue, 7 Nov 2023 16:06:35 -1000 Subject: [PATCH] Correct copy/move for `union` By writing separate construction and assignment, plus the new feature of suppressing assignment to a member by writing `member = _ ;` (now allowed only in assignment operators). I do realize that's an "opt-out" which I normally prefer to avoid, but: - I considered and decided against (for now) the alternative of not having assignment be memberwise by default. I want to keep the (new to Cpp2) default of memberwise semantics for assignment as with construction. I think that's a useful feature, and normally if you do assign to a member it doesn't arise, and so I think it makes sense to explicitly call out when we're choosing not to do any assignment at all to a member before doing other assignment processing. We'll get experience with how it goes. - `_` is arguably natural here, since it's pronounced "don't care." There too, we'll see if that is natural generalized, or feels strained. For now it feels natural to me. --- .../test-results/gcc-13/gcc-version.output | 2 +- regression-tests/test-results/pure2-union.cpp | 62 +++++++------------ regression-tests/test-results/version | 2 +- source/build.info | 2 +- source/cppfront.cpp | 29 ++++++++- source/reflect.h | 38 +++++++----- source/reflect.h2 | 22 ++++--- 7 files changed, 91 insertions(+), 66 deletions(-) diff --git a/regression-tests/test-results/gcc-13/gcc-version.output b/regression-tests/test-results/gcc-13/gcc-version.output index e1e0ea2bd..6635e5573 100644 --- a/regression-tests/test-results/gcc-13/gcc-version.output +++ b/regression-tests/test-results/gcc-13/gcc-version.output @@ -1,4 +1,4 @@ -gcc (GCC) 13.2.1 20230728 (Red Hat 13.2.1-1) +gcc (GCC) 13.2.1 20231011 (Red Hat 13.2.1-4) Copyright (C) 2023 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. diff --git a/regression-tests/test-results/pure2-union.cpp b/regression-tests/test-results/pure2-union.cpp index 543f70fe2..f74851b2d 100644 --- a/regression-tests/test-results/pure2-union.cpp +++ b/regression-tests/test-results/pure2-union.cpp @@ -35,8 +35,8 @@ public: ~name_or_number() noexcept; public: explicit name_or_number(); public: name_or_number(name_or_number const& that); -public: auto operator=(name_or_number const& that) -> name_or_number& ; public: name_or_number(name_or_number&& that) noexcept; +public: auto operator=(name_or_number const& that) -> name_or_number& ; public: auto operator=(name_or_number&& that) noexcept -> name_or_number& ; #line 5 "pure2-union.cpp2" @@ -62,8 +62,8 @@ private: auto _destroy() & -> void; public: ~name_or_other() noexcept; public: explicit name_or_other(); public: name_or_other(name_or_other const& that); -public: auto operator=(name_or_other const& that) -> name_or_other& ; public: name_or_other(name_or_other&& that) noexcept; +public: auto operator=(name_or_other const& that) -> name_or_other& ; public: auto operator=(name_or_other&& that) noexcept -> name_or_other& ; #line 17 "pure2-union.cpp2" @@ -101,39 +101,30 @@ auto name_or_number::_destroy() & -> void{ } name_or_number::~name_or_number() noexcept{_destroy();} -name_or_number::name_or_number() - : _discriminator{ -1 }{} +name_or_number::name_or_number(){} name_or_number::name_or_number(name_or_number const& that) - : _storage{ that._storage } - , _discriminator{ that._discriminator }{ + : _storage{ } + , _discriminator{ -1 }{ if (CPP2_UFCS_0(is_name, that)) {set_name(CPP2_UFCS_0(name, that));} if (CPP2_UFCS_0(is_num, that)) {set_num(CPP2_UFCS_0(num, that));} - _discriminator = that._discriminator; + } + + name_or_number::name_or_number(name_or_number&& that) noexcept + : _storage{ } + , _discriminator{ -1 }{ + if (CPP2_UFCS_0(is_name, std::move(that))) {set_name(CPP2_UFCS_0(name, std::move(that)));} + if (CPP2_UFCS_0(is_num, std::move(that))) {set_num(CPP2_UFCS_0(num, std::move(that)));} } auto name_or_number::operator=(name_or_number const& that) -> name_or_number& { - _storage = that._storage; - _discriminator = that._discriminator; if (CPP2_UFCS_0(is_name, that)) {set_name(CPP2_UFCS_0(name, that));} if (CPP2_UFCS_0(is_num, that)) {set_num(CPP2_UFCS_0(num, that));} - _discriminator = that._discriminator; return *this; } - name_or_number::name_or_number(name_or_number&& that) noexcept - : _storage{ std::move(that)._storage } - , _discriminator{ std::move(that)._discriminator }{ - if (CPP2_UFCS_0(is_name, std::move(that))) {set_name(CPP2_UFCS_0(name, std::move(that)));} - if (CPP2_UFCS_0(is_num, std::move(that))) {set_num(CPP2_UFCS_0(num, std::move(that)));} - _discriminator = std::move(that)._discriminator; - } - auto name_or_number::operator=(name_or_number&& that) noexcept -> name_or_number& { - _storage = std::move(that)._storage; - _discriminator = std::move(that)._discriminator; if (CPP2_UFCS_0(is_name, std::move(that))) {set_name(CPP2_UFCS_0(name, std::move(that)));} if (CPP2_UFCS_0(is_num, std::move(that))) {set_num(CPP2_UFCS_0(num, std::move(that)));} - _discriminator = std::move(that)._discriminator; return *this; } #line 12 "pure2-union.cpp2" @@ -166,40 +157,31 @@ template auto name_or_other::_destroy() & -> void{ } template name_or_other::~name_or_other() noexcept{_destroy();} -template name_or_other::name_or_other() - : _discriminator{ -1 }{} +template name_or_other::name_or_other(){} template name_or_other::name_or_other(name_or_other const& that) - : _storage{ that._storage } - , _discriminator{ that._discriminator }{ + : _storage{ } + , _discriminator{ -1 }{ if (CPP2_UFCS_0(is_name, that)) {set_name(CPP2_UFCS_0(name, that));} if (CPP2_UFCS_0(is_other, that)) {set_other(CPP2_UFCS_0(other, that));} - _discriminator = that._discriminator; } + template name_or_other::name_or_other(name_or_other&& that) noexcept + : _storage{ } + , _discriminator{ -1 }{ + if (CPP2_UFCS_0(is_name, std::move(that))) {set_name(CPP2_UFCS_0(name, std::move(that)));} + if (CPP2_UFCS_0(is_other, std::move(that))) {set_other(CPP2_UFCS_0(other, std::move(that)));} + } + template auto name_or_other::operator=(name_or_other const& that) -> name_or_other& { - _storage = that._storage; - _discriminator = that._discriminator; if (CPP2_UFCS_0(is_name, that)) {set_name(CPP2_UFCS_0(name, that));} if (CPP2_UFCS_0(is_other, that)) {set_other(CPP2_UFCS_0(other, that));} - _discriminator = that._discriminator; return *this; } - template name_or_other::name_or_other(name_or_other&& that) noexcept - : _storage{ std::move(that)._storage } - , _discriminator{ std::move(that)._discriminator }{ - if (CPP2_UFCS_0(is_name, std::move(that))) {set_name(CPP2_UFCS_0(name, std::move(that)));} - if (CPP2_UFCS_0(is_other, std::move(that))) {set_other(CPP2_UFCS_0(other, std::move(that)));} - _discriminator = std::move(that)._discriminator; - } - template auto name_or_other::operator=(name_or_other&& that) noexcept -> name_or_other& { - _storage = std::move(that)._storage; - _discriminator = std::move(that)._discriminator; if (CPP2_UFCS_0(is_name, std::move(that))) {set_name(CPP2_UFCS_0(name, std::move(that)));} if (CPP2_UFCS_0(is_other, std::move(that))) {set_other(CPP2_UFCS_0(other, std::move(that)));} - _discriminator = std::move(that)._discriminator; return *this; } #line 19 "pure2-union.cpp2" diff --git a/regression-tests/test-results/version b/regression-tests/test-results/version index df9b70efa..d06fda816 100644 --- a/regression-tests/test-results/version +++ b/regression-tests/test-results/version @@ -1,5 +1,5 @@ -cppfront compiler v0.3.0 Build 8A21:1546 +cppfront compiler v0.3.0 Build 8A27:1514 Copyright(c) Herb Sutter All rights reserved SPDX-License-Identifier: CC-BY-NC-ND-4.0 diff --git a/source/build.info b/source/build.info index 0e86d1e74..7a7c6158b 100644 --- a/source/build.info +++ b/source/build.info @@ -1 +1 @@ -"8A21:1546" \ No newline at end of file +"8A27:1514" \ No newline at end of file diff --git a/source/cppfront.cpp b/source/cppfront.cpp index cf1f08f3c..b193c8daa 100644 --- a/source/cppfront.cpp +++ b/source/cppfront.cpp @@ -4812,7 +4812,32 @@ class cppfront || found_default_init ); - // Emit the initializer... + // Emit the initializer if it it isn't '_' (don't care) and ... + if (initializer == "_") { + // I'll walk the walk: I've said publicly that _structured_ goto is perfectly + // kosher -- it's only _unstructured_ goto that's harmful (and Dijkstra knew it), + // which means: + // - jumping into a block or statement (I'm looking at you, Duff's Device) + // - jumping backwards (that's an unstructured loop == spaghetti control flow) + // - jumping across a variable declaration (C++ already bans this) + // + // But jumping forward-and-outward, skipping no declarations, is righteous. + // + // Here, using goto is the right tool for the job, and is better code because: + // - it avoids a gratuitous extra level of nesting inside an + // 'if (initializer != "_")' block of the next 100 lines (and please don't + // start the other diatribe about that the next 100 lines should be a + // separate named function - no it shouldn't) + // - which extra indent of identical code would make GitHub's diff for this + // commit super hard to read (diff does not deal well with blocks of code + // that simply change indentation level - in fact seeing that diff without + // the goto was the tipping point to just switch to goto here) + // ... but sadly I feel the need to defend it. + // + // As Scott would say (and has said), "so sue me" + // + goto skip_initializer; + } if (initializer.empty()) { initializer = "{}"; @@ -4909,6 +4934,8 @@ class cppfront separator = ", "; } + skip_initializer: ; + // And on to the next data member... ++object; } diff --git a/source/reflect.h b/source/reflect.h index 4e62c400f..9ea03adcd 100644 --- a/source/reflect.h +++ b/source/reflect.h @@ -38,7 +38,7 @@ class alias_declaration; #line 841 "reflect.h2" class value_member_info; -#line 1327 "reflect.h2" +#line 1335 "reflect.h2" } } @@ -699,14 +699,14 @@ auto flag_enum(meta::type_declaration& t) -> void; auto cpp2_union(meta::type_declaration& t) -> void; -#line 1213 "reflect.h2" +#line 1221 "reflect.h2" //----------------------------------------------------------------------- // // print - output a pretty-printed visualization of t // auto print(cpp2::in t) -> void; -#line 1223 "reflect.h2" +#line 1231 "reflect.h2" //----------------------------------------------------------------------- // // apply_metafunctions @@ -717,7 +717,7 @@ auto print(cpp2::in t) -> void; auto const& error ) -> bool; -#line 1327 "reflect.h2" +#line 1335 "reflect.h2" } } @@ -1680,14 +1680,14 @@ std::string destroy = " private _destroy: (inout this) = {\n"; // Add the destructor #line 1193 "reflect.h2" - CPP2_UFCS(add_member, t, " operator=: (move this) = { _destroy(); } "); + CPP2_UFCS(add_member, t, " operator=: (move this) = { _destroy(); }"); // Add default constructor - CPP2_UFCS(add_member, t, " operator=: (out this) = { _discriminator = -1; } "); + CPP2_UFCS(add_member, t, " operator=: (out this) = { }"); { -std::string value_set = " operator=: (out this, that) = {\n"; +std::string value_set = ""; - // Add value-set + // Add copy/move construction and assignment #line 1200 "reflect.h2" { @@ -1695,22 +1695,30 @@ std::string value_set = " operator=: (out this, that) = {\n"; auto const& a : alternatives ) { value_set += " if that.is_" + cpp2::to_string(a.name) + "() { set_" + cpp2::to_string(a.name) + "( that." + cpp2::to_string(a.name) + "() ); }\n"; } - - value_set += " _discriminator = that._discriminator;\n"; value_set += " }\n"; - CPP2_UFCS(add_member, t, std::move(value_set)); + + CPP2_UFCS(add_member, t, std::string(" operator=: (out this, that) = {\n") + + " _storage = ();\n" + + " _discriminator = -1;\n" + + value_set + ); + CPP2_UFCS(add_member, t, std::string(" operator=: (inout this, that) = {\n") + + " _storage = _;\n" + + " _discriminator = _;\n" + + std::move(value_set) + ); } } -#line 1210 "reflect.h2" +#line 1218 "reflect.h2" } -#line 1217 "reflect.h2" +#line 1225 "reflect.h2" auto print(cpp2::in t) -> void { std::cout << CPP2_UFCS_0(print, t) << "\n"; } -#line 1227 "reflect.h2" +#line 1235 "reflect.h2" [[nodiscard]] auto apply_metafunctions( declaration_node& n, type_declaration& rtype, @@ -1810,7 +1818,7 @@ auto print(cpp2::in t) -> void return true; } -#line 1327 "reflect.h2" +#line 1335 "reflect.h2" } } diff --git a/source/reflect.h2 b/source/reflect.h2 index 0db595192..3e842618c 100644 --- a/source/reflect.h2 +++ b/source/reflect.h2 @@ -1190,22 +1190,30 @@ union: (inout t : meta::type_declaration) } // Add the destructor - t.add_member( " operator=: (move this) = { _destroy(); } " ); + t.add_member( " operator=: (move this) = { _destroy(); }" ); // Add default constructor - t.add_member( " operator=: (out this) = { _discriminator = -1; } " ); + t.add_member( " operator=: (out this) = { }" ); - // Add value-set - (copy value_set: std::string = " operator=: (out this, that) = {\n") + // Add copy/move construction and assignment + (copy value_set: std::string = "") { for alternatives do (a) { value_set += " if that.is_(a.name)$() { set_(a.name)$( that.(a.name)$() ); }\n"; } - - value_set += " _discriminator = that._discriminator;\n"; value_set += " }\n"; - t.add_member( value_set ); + + t.add_member( std::string(" operator=: (out this, that) = {\n") + + " _storage = ();\n" + + " _discriminator = -1;\n" + + value_set + ); + t.add_member( std::string(" operator=: (inout this, that) = {\n") + + " _storage = _;\n" + + " _discriminator = _;\n" + + value_set + ); } }