[PATCH] D119927: [Clang] [P2025] More exhaustive tests for NRVO

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

Quuxplusone accepted this revision.
Quuxplusone added a comment.
This revision is now accepted and ready to land.

LGTM FWIW. You might want to wait for someone more authoritative to take a look; but it's also only adding test coverage, so it seems pretty uncontroversial, I would think.



================
Comment at: clang/test/CodeGenCXX/nrvo.cpp:165
   if (B)
-    return y;
-  return x;
+    return y; // NRVO is impossible
+  return x;   // NRVO is impossible
----------------
Technically, because `B` is a constant throughout this function, we probably //could// do NRVO here by hoisting the condition as if by
```
X x;
if (B) { X y; return y; } // NRVO is possible here
X y; return x; // NRVO is impossible
```
Whether we //should// is another question. :) Also, changing `bool B` to `const bool& B` would defeat my idea.


================
Comment at: clang/test/CodeGenCXX/nrvo.cpp:1149
+//
+void test16() {
+  X x;
----------------
It would be helpful to comment these tests also with their p2025 example numbers, e.g.
```
void test16() { // http://wg21.link/p2025r2#ex-9
```
It took me a while to realize that the numbers here don't actually match up to the examples' numbers in the paper.


================
Comment at: clang/test/CodeGenCXX/nrvo.cpp:1537
+  }
+  return x; // FIXME: NRVO could happen if B == false, but doesn't
+}
----------------
Nit: `s/if/when/`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119927/new/

https://reviews.llvm.org/D119927