Page MenuHomePhabricator

[C++20] P1825R0: More implicit moves
ClosedPublic

Authored by nullptr.cpp on Sep 24 2020, 5:18 AM.

Details

Summary

Changes in P1825R0:

  • implicitly movable entity can be an rvalue reference to non-volatile automatic object.
  • operand of throw-expression can be a function or catch-clause parameter (support for function parameter has already been implemented).
  • in the first overload resolution, the selected function no need to be a constructor.
  • in the first overload resolution, the first parameter of the selected function no need to be an rvalue reference to the object's type.

This patch also removes the diagnostic -Wreturn-std-move-in-c++11.

Diff Detail

Unit TestsFailed

TimeTest
80 mslinux > Clang.CXX/drs::dr15xx.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -std=c++98 -triple x86_64-unknown-unknown /mnt/disks/ssd0/agent/llvm-project/clang/test/CXX/drs/dr15xx.cpp -verify -fexceptions -fcxx-exceptions -pedantic-errors
170 mswindows > Clang.CXX/drs::dr15xx.cpp
Script: -- : 'RUN: at line 1'; c:\ws\w16-1\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w16-1\llvm-project\premerge-checks\build\lib\clang\12.0.0\include -nostdsysteminc -std=c++98 -triple x86_64-unknown-unknown C:\ws\w16-1\llvm-project\premerge-checks\clang\test\CXX\drs\dr15xx.cpp -verify -fexceptions -fcxx-exceptions -pedantic-errors

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
clang/lib/Sema/SemaStmt.cpp
3291

FWIW, I don't understand this TODO comment. What's to do, here?

clang/test/SemaCXX/implicit-move.cpp
21

if I understand correctly, you can (and should) remove the copy and move constructors from NeedRvalueRef and NoNeedRvalueRef.
Also, I strongly recommend renaming NoNeedRvalueRef to NeedValue (or something along those lines) — it's not that it doesn't need an rvalue ref, it's that it does need a non-ref.

Implement whole P1825R0

nullptr.cpp retitled this revision from [C++20] Implement more implicit moves for return statements(Part of P1825R0) to [C++20] P1825R0: More implicit moves.Nov 3 2020, 5:00 AM
nullptr.cpp edited the summary of this revision. (Show Details)
clang/include/clang/Sema/Sema.h
4488–4497

I believe RValue should be spelled Rvalue, throughout.

4496

Unless I'm mistaken, CES_AsIfByStdMove is now a synonym for CES_ImplicitlyMovableCXX20. Could we discuss the pros and cons of simply removing CES_AsIfByStdMove? Is there some difference in connotation here, like maybe we are expecting them to diverge again in C++23 or beyond?

clang/lib/Sema/SemaStmt.cpp
3123

This loop is hard to understand before your patch. I don't think you made it any worse. But, consider whether you could pull out the condition so that this part became something like

FunctionDecl *FD = Step.Function.Function;
if (!IsSuitableConversionForImplicitMove(FD, NRVOCandidate, ConvertingConstructorsOnly)) {
  break;
}
// Promote "AsRvalue" to the heap...

Actually, I don't even understand why continue; is being used in the old code. Doesn't that mean "skip this step and go on to the next step"?

clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
59 ↗(On Diff #304447)

(1) I would prefer to [additionally?] see a test case using =delete, rather than having the member exist but be private. We expect the same behavior in both cases, right? but =delete is more like the code we hope people are writing in practice.

(2) Is there any simpler way to write the "pre-C++20" and "C++20" versions of this test? It's ugly to repeat the entire class body in both branches of the #if, when the only difference is whether the "expected-note" is present or not.

92 ↗(On Diff #304447)

In all of these tests, I don't see any reason to have {} when ; would suffice; and I don't think you need the destructor to be explicitly declared at all; and in fact for NeedRvalueRef and NeedValue, I think just

struct NeedRvalueRef {
    NeedRvalueRef(B&&);
};

would suffice, right?

clang/test/SemaCXX/warn-return-std-move.cpp
87 ↗(On Diff #304447)

I would like to see a copy of this file, titled something like warn-return-std-move-cxx20.cpp, updated to run with -std=c++20 instead of -std=c++14. (Initially I thought that the lines above indicated a flaw in your patch; it took me a while to realize that this test file is compiled only with -std=c++14. In C++20, we expect that ConstructFromBase(Base&&) will be called and the expected-warning will not be printed, right?)

I would also appreciate either seeing an actual test file with all the test cases from http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1155r2.html , or at least hearing (in reply to this comment) that you've tested them all and you believe their behavior is correct.

It looks to me as if every P1155r2 case is handled correctly except for this one:

struct To {
    operator Widget() const &;  // "copy"
    operator Widget() &&;  // "move"
};
Widget nine() {
    To t;
    return t;  // C++17 copies; C++20 should move (but this patch still copies)
}
Quuxplusone added inline comments.
clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
22 ↗(On Diff #304447)

@rsmith @david_stone (or anyone), what is the status in C++20 of the following test case?

C&& test(C&& c) {
    return c;
}

I know we talked about this in person at CppCon 2018, and concluded that our intention was for this to be legal, but that it wasn't actually legal as-worded, because the returned thingie here is not an object but rather a reference, and therefore none of P1825's wording actually covers it. Is that still the case? Is there an open issue about this? Is there any appetite for Clang to just go ahead and make this legal? (The current patch does not make this legal.)

Relevant reading: https://quuxplusone.github.io/blog/2018/09/25/perfect-backwarding/

Quuxplusone added inline comments.
clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
22 ↗(On Diff #304447)

Apparently @davidstone has been active more recently than @david_stone... :)

davidstone added inline comments.Dec 2 2020, 4:46 PM
clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
22 ↗(On Diff #304447)

Going through the wording in http://eel.is/c++draft/class.copy.elision#3

"An implicitly movable entity is a variable of automatic storage duration that is either a non-volatile object or an rvalue reference to a non-volatile object type."

So we are fine with a reference as the source. However, it then goes on to say

"...overload resolution to select the constructor for the copy or the return_­value overload to call is first performed as if the expression or operand were an rvalue."

There is technically no constructor to call here, which I think means this section does not apply.

I don't believe an issue has been raised for this yet, so I'll email CWG about it.

I've found another case that Clang has never handled correctly (and which your patch does not fix)—
https://godbolt.org/z/xd8qGW

struct Widget {};
struct Frodo {
    Frodo(Widget&);
    Frodo(Widget&&) = delete;
};

Frodo twelve() {
    Widget w;
    return w;  // according to the majority of vendors, the first pass should not "fail";
               // it should successfully find the deleted overload and hard-error at that point.
               // This has been the case ever since C++11, AFAIK.
}

This example will be mentioned in my upcoming (not-yet-finished) WG21 paper P2266, as an example of why the two-pass mechanism sucks and should be removed from C++2b.

nullptr.cpp added inline comments.Dec 7 2020, 4:55 AM
clang/include/clang/Sema/Sema.h
4488–4497

There are already names such as RValueReferenceType, so be consistent with existing code.

clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
28 ↗(On Diff #309873)

[over.match.funcs]p8:

A defaulted move special member function that is defined as deleted is excluded from the set of candidate functions in all contexts.

So in this case still use private.

nullptr.cpp edited the summary of this revision. (Show Details)Dec 7 2020, 5:01 AM
nullptr.cpp edited the summary of this revision. (Show Details)
Quuxplusone added inline comments.Dec 7 2020, 5:48 AM
clang/include/clang/Sema/Sema.h
4488–4497

Okay, I see examples both ways and wish there were one standard spelling, but I agree there's no standard at the moment, so whatever.

clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
28 ↗(On Diff #309873)

If it's user-declared as deleted, then it is not defaulted.
To get a move-constructor that is defaulted as deleted, you'd have to write something like https://godbolt.org/z/KbG76v (Notice how the behavior changes if you explicitly delete it with C(C&&) = delete; instead of implicitly deleting it with C(C&&) = default;.)

22 ↗(On Diff #304447)

I don't know if there's a CWG issue number (yet?) but at least I have drafted a paper, D2266R0, on the subject. @nullptr.cpp could you take a look at this paper and let me know (perhaps by email) what you think about it?

Add more tests

ychen added a subscriber: ychen.Jan 12 2021, 7:03 PM

@nullptr.cpp: I may or may not come up with more review comments, but either way I'm a dead end — I don't consider myself authorized to "accept" Clang changes.
You're still going to have to attract the attention of someone with the authority and/or force-of-will to say "okay ship it."

I notice a lack of any explicitly co_return-related tests and/or code in this patch. I'm just going to assume that is fine.

nullptr.cpp added a comment.EditedJan 21 2021, 7:48 PM

I notice a lack of any explicitly co_return-related tests and/or code in this patch. I'm just going to assume that is fine.

co_return related implicit move is implemented by @aaronpuchert in D68845. Since P1825 mainly focused on removing the unnecessary limitations on implicit move, not coroutines, I think it's better to separate that from this patch.

Quuxplusone accepted this revision.Feb 16 2021, 8:12 AM

either way I'm a dead end — I don't consider myself authorized to "accept" Clang changes.
You're still going to have to attract the attention of someone with the authority and/or force-of-will to say "okay ship it."

Well, it's been a month since I said that. I'll land this tonight, unless someone yells "stop" before then. If it causes any problems, we can revert it easy enough.

This revision is now accepted and ready to land.Feb 16 2021, 8:12 AM

either way I'm a dead end — I don't consider myself authorized to "accept" Clang changes.
You're still going to have to attract the attention of someone with the authority and/or force-of-will to say "okay ship it."

Well, it's been a month since I said that. I'll land this tonight, unless someone yells "stop" before then. If it causes any problems, we can revert it easy enough.

I think you're much more of an expert on implicit moves than most of us (except @rsmith perhaps). I've done a superficial review and didn't find anything objectionable, also we're early in the release cycle, so I think this approach is fine.

I think you're much more of an expert on implicit moves than most of us

Short answer, yes. ;) My concern was more that I'm much less of an expert on Clang than "most of us." I'd rather trust a Clang expert to learn and implement the couple of pages of P1825, than you should trust a P1825 expert to learn and implement the couple hundred lines of Clang in this patch!

But as it's been months, I'm finally ready to WP:BOLD and let someone revert it later if they need to.

This revision was automatically updated to reflect the committed changes.

My concern was more that I'm much less of an expert on Clang than "most of us."

Understood, but Clang tries to closely follow the C++ standard in its implementation, and I'd say that the APIs used in this change do exactly what one would naively think they do. And we haven't introduced any new functions so we can't even discuss where to put them.

I'm sometimes reading up about implicit moving but the area seemed a bit in flux so I was hoping for things to settle down before I start to remember what might soon be obsolete. Let's hope this settles it.

The removal of -Wreturn-std-move-in-c++11 breaks a few projects. These projects are specifying the option, which then triggers an unknown-option warning and thus -Wx halts the build.

Would anyone object to allowing the option but silently ignoring it, at least for a transition period? Was there prior notice of this option being deprecated?

jgorbe added a subscriber: jgorbe.Feb 18 2021, 2:14 PM

Would anyone object to allowing the option but silently ignoring it, at least for a transition period?

No objection from me.

Was there prior notice of this option being deprecated?

I don't think we have a way to deprecate warning flags. We could perhaps put something under -Woption-ignored.

@aaron.ballman, what do you think about this?

Would anyone object to allowing the option but silently ignoring it, at least for a transition period?

No objection from me.

Nor from me.

Was there prior notice of this option being deprecated?

I don't think we have a way to deprecate warning flags. We could perhaps put something under -Woption-ignored.

@aaron.ballman, what do you think about this?

We don't have a loud way of deprecating them (that I'm aware of). We usually rely on the release notes to say something, but we didn't do that here. As for using -Woption-ignored... I am not certain we want it directly under that flag, but it may make sense as a subgroup of -Woption-ignored. e.g., I can imagine a use case where someone wants to silence that they're using a deprecated flag (so their -Wx builds continue to compile) but still wants to be warned when they use a option that's ignored for other reasons.

We usually rely on the release notes to say something, but we didn't do that here.

Perhaps @nullptr.cpp could add something there? The file is clang/docs/ReleaseNotes.rst.

As for using -Woption-ignored... I am not certain we want it directly under that flag, but it may make sense as a subgroup of -Woption-ignored. e.g., I can imagine a use case where someone wants to silence that they're using a deprecated flag (so their -Wx builds continue to compile) but still wants to be warned when they use a option that's ignored for other reasons.

You're right, the other flags -Woption-ignored warns about seem to have an effect on compilation such that one might want to be warned if they're ignored. Which isn't the case here, I guess.

Also it's probably Ok if people just see that the warning no longer exists and then remove it, or remove it based on the compiler version.

We usually rely on the release notes to say something, but we didn't do that here.

Perhaps @nullptr.cpp could add something there? The file is clang/docs/ReleaseNotes.rst.

Add by D97364.

The option is used in the wild, but not as widely as I first believed. We've already fixed a couple projects that were using it. I think the Release Note is the right solution. Thanks for doing that. I withdraw my suggestion to allow and ignore.

rsmith added inline comments.Mar 2 2021, 2:53 PM
clang/www/cxx_status.html
1204–1208

We have, so far, listed DR papers only in the section of cxx_status for the earliest language they affect (that is: the section for C++XY lists the papers that we implement in C++XY but not in earlier versions). I'm not tied to that approach, but if we want to switch away from that (and list DR papers multiple times) we should do so consistently throughout this page -- also, if we go that way, maybe the earlier reference to the paper should contain a link to the later one?

The preceding paper, P0593R6, is a bit of a special case because while -- per our "DRs apply retroactively as far back as they make sense" policy -- it should apply retroactively all the way back to C++98, it only actually affects Clang's behavior in C++20 mode, as part of P0784R7, which allowed pseudo-destruction in constant expressions. Maybe there's a clearer way to present that change.

sberg added a subscriber: sberg.Mar 19 2021, 8:25 AM

This change causes

#include <memory>
std::shared_ptr<int> x1;
std::shared_ptr<int> f() {
    std::shared_ptr<int> & r = x1;
    r.reset(new int);
    return r;
}
int main() {
    std::shared_ptr<int> x2 = f();
    long n = x2.use_count();
    return n;
}

when built with -std=c++20 to erroneously return a use_count of 1 instead of 2 now.

Confirmed: yikes. @nullptr.cpp, I'm going to open a new pull request to revert this, to poke the buildbot, just in case reverting it causes new failures. But I certainly don't object if someone wants to be more bold than me in reverting quickly.
Here's a reduced test case: https://godbolt.org/z/dz43W4

struct A {
    A(A&&);
    A(const A&);
};

extern A global;

A f() {
    A& r = global;
    return r;
}

To whom it may concern, see D98971. I'm (unwisely?) trying a surgical fix rather than trying to revert the month-old thing. (Let's please discuss anything D98971-specific over there.)

rsmith added inline comments.May 13 2021, 1:35 PM
clang/lib/Sema/SemaStmt.cpp
3202–3203

P1825 was accepted as a Defect Report (see https://wiki.edg.com/bin/view/Wg21cologne2019/StrawPolls); is there a reason we're not applying this retroactively?

clang/lib/Sema/SemaStmt.cpp
3202–3203

Move to accept the changes in P1825R0 (Merged wording for P0527R1 and P1155R3) as a Defect Report and apply them to the C++ working paper.

Yipes. @rsmith, is the intent that modern compilers should support

std::unique_ptr<int> from_rvalue(std::unique_ptr<int>&& x) { return x; }

in -std=c++11 mode? Are Clang, GCC, MSVC, EDG all on the same page here? If so, awesome. I just find it a priori unlikely that we got informed consensus on that.

rsmith added inline comments.May 13 2021, 2:49 PM
clang/lib/Sema/SemaStmt.cpp
3202–3203

Yes, the intent is that P1825 should be applied all the way back to C++11. See the CWG discussion here: https://wiki.edg.com/bin/view/Wg21cologne2019/CoreWorkingGroup#D1825R0_Merged_wording_for_P_AN1 -- in which we concluded that the entire paper should be treated as a DR.

Regarding implementation consensus: MSVC already implements the changes in all language modes (since v19.24). It looks like the intent is for GCC to do the same in GCC 12 onwards (https://github.com/gcc-mirror/gcc/commit/1722e2013f05f1f1f99379dbaa0c0df356da731f). I'm not sure what EDG's plans are.

I think for us the question is whether we can get away with doing this without any kind of transition period. If extending this all the way back to C++11 is disruptive in practice for existing code, we may need to take a more measured approach, such as phasing the introduction of this over a couple of Clang releases with warnings for things that will change behavior, but if not, I think we should just do it (and there's no better way to find out how disruptive this is than doing it...). We might actually need to implement P2266 as well, in order to avoid regressions such as the one that Jason mentioned in that GCC commit message; I suppose we'll find out.

I wonder whether we can extend this back to C++98 (and thereby have the same rules regardless of language standard). If someone is using rvalue references in C++98 (which we allow as an extension) it seems to make sense for the new behavior to apply to them. Are there cases where the new rules would affect the meaning of a standard C++98 program (ie, one that has no move constructors, no rvalue reference parameters, and so on)?

clang/lib/Sema/SemaStmt.cpp
3202–3203

From Godbolt: MSVC implements all parts of p1825 in C++17 mode.
From that patch, and Godbolt: GCC trunk implements some subtle variation of p1155 (but specifically no part of p0527) in C++17 mode.

Are there cases where the new rules would affect the meaning of a standard C++98 program (ie, one that has no move constructors, no rvalue reference parameters, and so on)?

Yes, Jason Merrill's example is specifically about p1155's effect on a pathological C++98 type, which I call AutoSharePtr in my C++Now 2021 talk "The Complete Guide to return x". I will start recommending that people watch that talk, as soon as it hits YouTube. :) https://godbolt.org/z/T374o518W

mizvekov added inline comments.
clang/lib/Sema/SemaStmt.cpp
3202–3203

The most churn from this change should be caused by updating the tests.
Fortunately, we did this cleanup just a while back which should help a lot: D99225, which covers everything NRVO related in clang.

@rsmith we have a working implementation of P2266: D99005 (which depends on D99696 due to shared cleanups).
What we miss on that is implementing warnings to older standards about those changes, which we need to reevaluate anyway due to how much stuff is being changed and DR'd here.

By the way, also about what Jason Merril said in that commit: "Discussion on the CWG reflector ... settled on the model of doing only a single overload resolution, with the variable treated as an xvalue that can bind to non-const lvalue references."

^ That is what I implemented, if by any chance I understood correctly, here: D100000 (which depends on the P2266 DR above)
I am not set on any differences between my approach and the one Jason implemented, I mostly care about reducing this to a single overload resolution as welll...
So any input on that ^ would be very much appreciated.