Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Excellent!
| clang/test/CodeGenCXX/nrvo.cpp | ||
|---|---|---|
| 618 | 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. (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.) | |
| 1600 | E.g. here, IIUC, I would comment this as return x; // NRVO happens | |
Thanks! Yes I should've write comments that are understandable not only for me =)
I added comments to the existing tests as well.
Though NRVO attribute is bound to the variable, I'm also more comfortable to place comments in the "return" lines, as it is looks somewhat more clear.
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.
| clang/test/CodeGenCXX/nrvo.cpp | ||
|---|---|---|
| 164–165 | 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 impossibleWhether we should is another question. :) Also, changing bool B to const bool& B would defeat my idea. | |
| 1148 | It would be helpful to comment these tests also with their p2025 example numbers, e.g. void test16() { // http://wg21.link/p2025r2#ex-9It took me a while to realize that the numbers here don't actually match up to the examples' numbers in the paper. | |
| 1536 | Nit: s/if/when/ | |
I placed links to corresponding p2025 examples.
Some of the examples are reasonably absent from the test, such as 1st (RVO, not NRVO), 13th (consteval methods are not getting to LLVM IR), 17th (there are no NRVOs for non-class types in Clang), etc.
The 19th example (about volatiles) is massive and has three corresponding functions in the test.
| clang/test/CodeGenCXX/nrvo.cpp | ||
|---|---|---|
| 164–165 | I have been thinking on "sorting" variables within the scope to get the optimal NRVO condition (right after seeing test2), but didn't come up with anything =( I guess that it's not legal at all, because initializing constructors (unlike copy/move constructors) tend to have meaningful actions (starting a timer, etc.), and many things will break badly if we will shift the declarations as we want. | |
@Quuxplusone Thanks for reviewing the patch! We can wait some time if someone else wants to take a look. Though I doubt if there may be major complaints on extendind the tests (especially with comments and references to a proposal).
Let me copy-paste here a standard disclaimer =) Should've done it right away, but I forgot about it.
P.S. If this review is eventually approved, kindly please merge the commit on my behalf =) As I don't have merge access. My name is Evgeny Shulgin and email is izaronplatz@gmail.com. Sorry for inconvenience!
| clang/test/CodeGenCXX/nrvo.cpp | ||
|---|---|---|
| 1536 | Serendipitously, I just ran into almost this exact scenario in D120180, where C++20's reverse_iterator wants to do basically {
_Iter __tmp = current;
--__tmp;
if constexpr (is_pointer_v<_Iter>) {
return __tmp;
} else {
return std::move(__tmp).operator->();
}
}and so we want NRVO on __tmp in the former case but URVO in the latter case. Of course in that specific case, we "want NRVO" only when __tmp is a pointer and thus NRVO doesn't apply anyway because pointers are returned in registers. But it would be nice to have a test case for as-close-as-possible to that pattern, if you don't mind adding one. | |
@Quuxplusone, thank you for the test idea! I added test25 that resembles the pattern.
Could you please look if we're okay with test25?
If everything is OK, I could merge the patch myself: I've got repository merge access =)
P. S. Sorry for the long delay, I was really busy lately.
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 impossibleWhether we should is another question. :) Also, changing bool B to const bool& B would defeat my idea.