Skip to content

Commit

Permalink
Correct copy/move for union
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
hsutter committed Nov 8, 2023
1 parent 083c8a0 commit cdf71bd
Show file tree
Hide file tree
Showing 7 changed files with 91 additions and 66 deletions.
2 changes: 1 addition & 1 deletion regression-tests/test-results/gcc-13/gcc-version.output
Original file line number Diff line number Diff line change
@@ -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.
Expand Down
62 changes: 22 additions & 40 deletions regression-tests/test-results/pure2-union.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -166,40 +157,31 @@ template <typename T> auto name_or_other<T>::_destroy() & -> void{
}

template <typename T> name_or_other<T>::~name_or_other() noexcept{_destroy();}
template <typename T> name_or_other<T>::name_or_other()
: _discriminator{ -1 }{}
template <typename T> name_or_other<T>::name_or_other(){}
template <typename T> name_or_other<T>::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 <typename T> name_or_other<T>::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 <typename T> auto name_or_other<T>::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 <typename T> name_or_other<T>::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 <typename T> auto name_or_other<T>::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"
Expand Down
2 changes: 1 addition & 1 deletion regression-tests/test-results/version
Original file line number Diff line number Diff line change
@@ -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
Expand Down
2 changes: 1 addition & 1 deletion source/build.info
Original file line number Diff line number Diff line change
@@ -1 +1 @@
"8A21:1546"
"8A27:1514"
29 changes: 28 additions & 1 deletion source/cppfront.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)

This comment has been minimized.

Copy link
@jcanizales

jcanizales Nov 8, 2023

Oh, the "hide whitespace only changes" feature is only implemented in pull requests?

This comment has been minimized.

Copy link
@JohelEGP

JohelEGP Nov 8, 2023

Contributor

That's right.

This comment has been minimized.

Copy link
@JohelEGP

JohelEGP Nov 8, 2023

Contributor

With git, you can use git diff -w/git log -w, and some more complex ones.

This comment has been minimized.

Copy link
@gregmarr

gregmarr Nov 8, 2023

Contributor

@hsutter Since you explained your thought process here, I'm curious about your choice of adding the label and goto which you thought required an explanation vs

++object;
continue;

Is this just future proofing in case something else would be needed at the end of the loop in the future?

I was also wondering why you needed the ++object at the end instead of in the loop statement, but then I found this in another part of the loop, and then it made sense:

++statement;
continue;

This comment has been minimized.

Copy link
@hsutter

hsutter Nov 8, 2023

Author Owner

I'm curious about your choice of adding the label and goto which you thought required an explanation vs

Because, er, um, I didn't notice that. :) Thanks! Fixed: 61b9b1c

Longer answer: While I was writing the code, I was moving the jump target around, and I didn't notice that it ended up pretty much the end of the loop body (the loop body didn't fit on a screen, so I suppose this is an argument favoring short loop bodies).

This comment has been minimized.

Copy link
@gregmarr

gregmarr Nov 8, 2023

Contributor

That makes sense. I suppose you were also a bit busy this week.

This comment has been minimized.

Copy link
@hsutter

hsutter Nov 9, 2023

Author Owner

I actually wrote that code on a plane nearly two weeks ago, just never had time to push the commit :) and wasn't planning to look at it again until after Kona.

But then I had to commit it in order to get to implementing the contracts natural syntax yesterday, which I had reason to look at yesterday after conversations with the authors

This comment has been minimized.

Copy link
@JohelEGP

JohelEGP Nov 29, 2023

Contributor

Oh, the "hide whitespace only changes" feature is only implemented in pull requests?

Now it's up there for commits, too.

// ... 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 = "{}";
Expand Down Expand Up @@ -4909,6 +4934,8 @@ class cppfront
separator = ", ";
}

skip_initializer: ;

// And on to the next data member...
++object;
}
Expand Down
38 changes: 23 additions & 15 deletions source/reflect.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class alias_declaration;
#line 841 "reflect.h2"
class value_member_info;

#line 1327 "reflect.h2"
#line 1335 "reflect.h2"
}

}
Expand Down Expand Up @@ -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<meta::type_declaration> t) -> void;

#line 1223 "reflect.h2"
#line 1231 "reflect.h2"
//-----------------------------------------------------------------------
//
// apply_metafunctions
Expand All @@ -717,7 +717,7 @@ auto print(cpp2::in<meta::type_declaration> t) -> void;
auto const& error
) -> bool;

#line 1327 "reflect.h2"
#line 1335 "reflect.h2"
}

}
Expand Down Expand Up @@ -1680,37 +1680,45 @@ 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"
{
for (
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<meta::type_declaration> 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,
Expand Down Expand Up @@ -1810,7 +1818,7 @@ auto print(cpp2::in<meta::type_declaration> t) -> void
return true;
}

#line 1327 "reflect.h2"
#line 1335 "reflect.h2"
}

}
Expand Down
22 changes: 15 additions & 7 deletions source/reflect.h2
Original file line number Diff line number Diff line change
Expand Up @@ -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"

This comment has been minimized.

Copy link
@gregmarr

gregmarr Nov 8, 2023

Contributor

@hsutter Do I understand this correctly that you have to start with _storage = _; to suppress memberwise assignment before the rest of the body because you're not just assigning directly to the member, and you're using conditionals to select what to assign?

This comment has been minimized.

Copy link
@JohelEGP

JohelEGP Nov 8, 2023

Contributor

It's to suppress memberwise assignment,
regardless of whether or not it's assigned to below.

This comment has been minimized.

Copy link
@gregmarr

gregmarr Nov 8, 2023

Contributor

I know what it's doing, I'm asking if it was necessary because the next code was like this:

if (something()) {
    _storage = foo;
} else {
    _storage = bar;
}

rather than

_storage = that._storage;

Edit: rephrased to make it a bit clearer.

This comment has been minimized.

Copy link
@JohelEGP

JohelEGP Nov 8, 2023

Contributor

No.
This works, compiles, and runs fine:

t: type = {
  public v: int = 0;
  operator=: (out this, x: int) = v = x;
  operator=: (inout this, x: int) = v = _;
}
main: () = {
  a: t = (42);
  assert(a.v == 42);
  a = 1729;
  assert(a.v == 42);
}

This comment has been minimized.

Copy link
@gregmarr

gregmarr Nov 8, 2023

Contributor

This works, compiles, and runs fine:

What does that example have to do with suppressing default memberwise assignment from that?

This comment has been minimized.

Copy link
@JohelEGP

JohelEGP Nov 8, 2023

Contributor

I know what it's doing, I'm asking if it was necessary because the next code was like this:

The alternative isn't correct.
And not suppressing the assignment would redundantly copy the storage.

This comment has been minimized.

Copy link
@gregmarr

gregmarr Nov 8, 2023

Contributor

And not suppressing the assignment would redundantly copy the storage.

I understand that. I'm confirming my understanding of the rules. WHEN do I have to suppress the default assignment? Do I always have to manually suppress the assignment first using _storage = _; or will any unconditional assignment suppress it?

In other words, does this ALSO suppress the default assignment?

    _storage = blah;

or do I need to do this?

    _storage = _;
    _storage = blah;

I suspect that _storage = blah; is enough. If that is enough, why is this not enough?

    if (something()) {
        _storage = foo;
    } else {
        _storage = bar; 
    }

Is it just because it's too complex?

I think that there is a rule "The first statements of the function must be assignments to the member variables, and they must appear in order. If they don't, then the default assignment will be inserted." That tells me that this would then work.

    _storage = something() ? foo : bar;

For union, the initialization selection is very complex, and thus it requires the new syntax of _storage = _; to say "Trust me and don't do the memberwise init, I'm going to initialize this later in the function using more complex code."

This comment has been minimized.

Copy link
@hsutter

hsutter Nov 8, 2023

Author Owner

Is it just because it's too complex?

I think that there is a rule "The first statements of the function must be assignments to the member variables, and they must appear in order. If they don't, then the default assignment will be inserted."

Exactly. I'm considering using the definite-first-use logic for these initializations/assignments too (which would allow that if), and a lot of the machinery for that is already enabled, but I haven't gone that far yet.

And in general if the initialization is complex in cases like union it seemed the general member = _; would be useful for such types' assignment operators (whether or not generated by a metafunction).

As the commit says, it was that or drop the default memberwise assignment, and I think the default memberwise assignment is valuable and nearly always correct, and that at this point it seems reasonable to require the programmer to call out "no I'm not doing that." Experience will tell, though!

This comment has been minimized.

Copy link
@gregmarr

gregmarr Nov 8, 2023

Contributor

Thanks, that confirms my initial reaction.

This comment has been minimized.

Copy link
@JohelEGP

JohelEGP Nov 8, 2023

Contributor

Is it just because it's too complex?
I think that there is a rule "The first statements of the function must be assignments to the member variables, and they must appear in order. If they don't, then the default assignment will be inserted."

Exactly. I'm considering using the definite-first-use logic for these initializations/assignments too (which would allow that if), and a lot of the machinery for that is already enabled, but I haven't gone that far yet.

That would resolve #475.

+ value_set
);
}
}

Expand Down

0 comments on commit cdf71bd

Please sign in to comment.