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

Arthur O'Dwyer via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue May 22 13:36:54 PDT 2018

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?

  rC Clang


More information about the cfe-commits mailing list