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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
