[PATCH] D47067: Update NRVO logic to support early return

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
Comment at: lib/Sema/SemaDecl.cpp:12760
       // to deduce an implicit return type.
-      if (FD->getReturnType()->isRecordType() &&
-          (!getLangOpts().CPlusPlus || !FD->isDependentContext()))
+      if (!FD->getReturnType()->isScalarType())
         computeNRVO(Body, getCurFunction());
----------------
What is the purpose of this change?
If it's "no functional change" it should be done separately IMHO; if it is supposed to change codegen, then it needs some codegen tests. (From looking at the code: maybe this affects codegen on functions that return member function pointers by value?)


================
Comment at: test/CodeGenCXX/nrvo.cpp:139
   }
-  // FIXME: we should NRVO this variable too.
-  X x;
+  // CHECK: tail call {{.*}} @_ZN1XC1ERKS_
   return x;
----------------
You've changed this function from testing one thing with a FIXME, to testing a completely different thing.  Could you add your new code as a new function, and leave the old FIXME alone until it's fixed?
Alternatively, if your patch actually fixes the FIXME, could you just replace the FIXME comment with a CHECK and leave the rest of this function alone?


================
Comment at: test/CodeGenCXX/nrvo.cpp:254
+  T t;
+  return t;
+}
----------------
Just for my own curiosity: this new test case is surely unaffected by this patch, right?


Repository:
  rC Clang

https://reviews.llvm.org/D47067