andreasfertig / cppinsights

C++ Insights - See your source code with the eyes of a compiler
https://cppinsights.io
MIT License
4.04k stars 238 forks source link

NRVO translation is not NRVO eligible #620

Closed rmccampbell closed 5 months ago

rmccampbell commented 5 months ago

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 commented 5 months ago

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

rmccampbell commented 5 months ago

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)?