[PATCH] D119927: [Clang] [P2025] More exhaustive tests for NRVO
Arthur O'Dwyer via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 16 09:12:01 PST 2022
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
More information about the cfe-commits
mailing list