Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[class.copy] Rephrase rule preferring a move constructor #1096

Merged
merged 1 commit into from Dec 14, 2016

Conversation

jensmaurer
Copy link
Member

in a throw-expression or a return statement.

Fixes #712.

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 21, 2016

@zygoloid: This looks like your area.

@@ -3231,17 +3231,20 @@
\end{example}

\pnum
When the criteria for elision of a copy/move operation are met,
Overload resolution to select the constructor
for the copy is first performed as if the object were designated by an
Copy link
Member

@zygoloid zygoloid Nov 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This "the copy" is missing an antecedent. I think we need this text to still be positioned after the bulleted list, since it's discussing a copy that is determined by the contents of the bullets. The content of the first bullet is still awkward. Something like this formulation would seem a lot clearer:

In the following cases, a copy may be implicitly promoted to a move:

  • if the expression in a return statement is a (possibly parenthesized) id-expression that names an object with automatic storage duration declared in the body or parameter-declaration-clause of the innermost enclosing function or lambda-expression, or
  • if the operand of a throw-expression is the name of a non-volatile automatic object (other than a function or catch-clause parameter) whose scope does not extend beyond the end of the innermost enclosing try-block (if there is one).

In each case, overload resolution to select the constructor for the copy is first performed as if the object were designated by an rvalue.

We should also point out to core that the second case should probably be changed to simply:

  • if the operand of a throw-expression is the name of an automatic object whose scope does not extend beyond the end of the innermost enclosing try-block (if there is one).

... for consistency with what we do for return statements. But that change would obviously not be editorial.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jensmaurer: Ping.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tkoeppe: Pong.

@jensmaurer
Copy link
Member Author

Updated.

@tkoeppe
Copy link
Contributor

tkoeppe commented Dec 14, 2016

This needs to be rebased for some reason.

@jensmaurer
Copy link
Member Author

Rebased.

and the object
to be copied is designated by an lvalue,
or when the \grammarterm{expression} in a \tcode{return} statement
In the following copy-initialization contexts, a move constructor might be used instead of a copy constructor:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is entirely accurate: the constructor we end up using might not be a move constructor (in particular, it might be instantiated from a constructor template). Can we make this a little more vague (given that the precise rules are later)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe:

"In the following copy-initialization contexts, a move operation might be used instead of a copy operation:"

We say "A copy operation [...] may be [...] converted to a move operation" in [stmt.return]/2, and mention copy/move operations in [class.copy.elision]/1 and in a few other contexts where we don't necessarily mean a copy/move constructor is used, but do mean a constructor that's constructing from another object of the same type, so this seems like consistent use of terminology (even if we don't define the term).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


\item if the operand of a \grammarterm{throw-expression}~(\ref{expr.throw})
is the name of a non-volatile automatic object
(other than a function or catch-clause parameter)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I agree this is a correct rephrasing of the rule, but it sure does seem strange that this would not apply to function and catch-clause parameters. May be worth filing a CWG issue, even if it only gets dumped to P3.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have a try...catch in the same function, we surely can't move away the function parameter or catch-clause. For the function parameter, we could say that we need to be in the outermost try-block (if any) to be able to move from a function parameter. For a cache-clause parameter, maybe the requirement "scope does not extend beyond..." already covers it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the "scope does not extend beyond" rule would already do the right thing in all the relevant cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful. Every possible combination of can/cannot move function parameters and can/cannot move catch-clause parameters is provided by one of the big four implementations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zygoloid: Isn't that what they mean by "has it been implemented"? :-)

is the name of a non-volatile automatic object
(other than a function or catch-clause parameter)
whose scope does not extend beyond the end of the innermost enclosing
\grammarterm{try-block} (if there is one),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also seems like a defect (perhaps part of the same one): the try-block includes the handlers, which means you don't get to do copy-to-move conversion in this case, where it would obviously be safe:

void f() {
  try { /*...*/ }
  catch (my_exception a) {
    if (cond) throw a; // no move here =(
    my_wrapper_exception b(a);
    throw b; // no move here either =(
  }
}

There doesn't seem to be a technical reason to not move in the above cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be dense here, but "b" is an automatic object whose scope does not extend beyond the innermost enclosing "try", right? So why don't I get a move for "b"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Send e-mail to -core pointing out these issues.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Err, right, bad example, a correct example would be more like this:

void f() {
  exception_thrown_on_failure f;
  try { /*...*/ }
  catch (...) {
    throw f; // no move here =(
  }
}

I see no reason we couldn't use a move here. Anyway, yes, I'll take this to the core reflector.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in a throw-expression or a return statement.

Fixes cplusplus#712.
@jensmaurer
Copy link
Member Author

Updated for "copy/move operation".

@zygoloid zygoloid merged commit 32026a5 into cplusplus:master Dec 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants