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

NRVO translation is not NRVO eligible #620

Closed
rmccampbell opened this issue Mar 31, 2024 · 2 comments
Closed

NRVO translation is not NRVO eligible #620

rmccampbell opened this issue Mar 31, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@rmccampbell
Copy link

When given a function invoking NRVO, for example:

C f() {
  C c;
  return c;
}

The insight returns

C f()
{
  C c = C() /* NRVO variable */;
  return C(static_cast<C &&>(c));
}

If I substitute this directly in the code however, the compiler can no longer apply NRVO and instead calls the move constructor on c. See https://godbolt.org/z/qecYW1e9e. When returning c directly, there is no move constructor and only one destructor called.

Note however that if c is a parameter then the move construction is necessary, so this seems to be a correct translation in that case, e.g.

C f(C c) {
  return c; // return C(static_cast<C &&>(c));
}
@andreasfertig andreasfertig added the bug Something isn't working label Apr 2, 2024
@andreasfertig
Copy link
Owner

Hello @rmccampbell,

thanks for starting this conversation!

NRVO is tricky. It is an optimization performed by the compiler in the back end. C++ Insights looks at the front end. The thing that plays the major role here is guaranteed copy elison.

What C++ Insights shows is what Clang tells me. In the initial AST (https://godbolt.org/z/v8brhz16E) you see:

|   `-ReturnStmt <line:19:5, col:12> nrvo_candidate(Var 0xba30518 'c' 'C')
|     `-CXXConstructExpr <col:12> 'C' 'void (C &&)'
|       `-ImplicitCastExpr <col:12> 'C' xvalue <NoOp>
|         `-DeclRefExpr <col:12> 'C' lvalue Var 0xba30518 'c' 'C'

Notice that it says nrvo_candidate at this point. The compiler may decide differently. However, I will use that information and keep the variable in that case. A fix is on its way.

Andreas

andreasfertig added a commit that referenced this issue Apr 2, 2024
Fixed #620: Do not show move construction for NRVO return.
@rmccampbell
Copy link
Author

I wonder if it would make sense to add another comment to the return statement to emphasize that NRVO is happening (in case the variable declaration is far away)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants