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

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

Quuxplusone added a comment.

Excellent!



================
Comment at: clang/test/CodeGenCXX/nrvo.cpp:622
+  if (b)
+    return x;
+  else
----------------
Similar to what you did in `test5`, I think it'd be helpful to comment //on the return line itself// whether NRVO is (done|possible|impossible). E.g.
`return x; // NRVO is impossible`
or
`return x; // FIXME: NRVO could happen, but doesn't`
or
`return x; // NRVO happens`
Your existing comment "No NRVO" is ambiguous — you mean it //doesn't// happen, or it //can't possibly// happen? — and makes me think just a little more than I ought to, because I have to skim the code and figure out which variable(s) and which return statement(s) we're talking about.

(I can see it being arguable whether the comment should go on the `return x;` or on the `X x;`. Me personally, I'd put it on the `return`. But I don't care enough to argue if someone thinks it should go on the variable definition instead.)


================
Comment at: clang/test/CodeGenCXX/nrvo.cpp:1604
+  X x;
+  return x;
+}
----------------
E.g. here, IIUC, I would comment this as
```
return x; // NRVO happens
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119927