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
Conversation
@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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jensmaurer: Ping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tkoeppe: Pong.
b5439d9
to
5e4bce3
Compare
Updated. |
This needs to be rebased for some reason. |
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: |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Updated for "copy/move operation". |
in a throw-expression or a return statement.
Fixes #712.