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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I'd like to see some indication in the code comments, in the commit message, and/or in cxx_status.html, explaining what parts of P1825 are implemented and what parts remain unimplemented after this patch. (My ideal would be to hold this patch up until it completely implements P1825. That way we minimize the number of minor divergences in implementation quality that people will have to memorize: Clang 10 does X, Clang 11 does Y, Clang 12 does Z... let's just jump straight from X to Z if at all humanly possible.)

clang/lib/Sema/SemaStmt.cpp
3091

This looks like a severable bugfix, right? Clang's current behavior diverges from GCC's, when the named return variable is volatile-qualified. https://godbolt.org/z/5EfK99
Personally I think this should be a separate bugfix patch, with regression test, fast-tracked separately from any of the C++20 stuff.

3115–3117

You changed "p32" to "p3:" — I think you meant "p3" without the colon.

3228

FWIW, I really like the idea of this table, but I don't know how to read it in this format. Are the papers/issues listed below the relevant standard, or above it?

My understanding is that CWG1579 is a defect report against C++11 (so, -std=c++11 mode includes the new behavior added in CWG1579), but for example -std=c++17 mode definitely does not include the new behavior for rvalue references added in P1825.

Could you change e.g. "C++11" to "-std=c++11 mode includes the following patches:", for clarity?

3232

For the record, I am the person who added the diagnostic controlled by AffectedByCWG1579, and my opinion is that if removing that diagnostic would simplify the code, you should consider it. We should certainly remove it from Clang at some point — this year? next year? 2025? but it should eventually be removed.

3283

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

clang/test/SemaCXX/implicit-move.cpp
21 ↗(On Diff #294024)

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)
Quuxplusone added inline comments.Nov 23 2020, 8:43 AM
clang/include/clang/Sema/Sema.h
4727–4732

I believe RValue should be spelled Rvalue, throughout.

4735

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
3159

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

(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

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
92

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

@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

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

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
4727–4732

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

[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
4727–4732

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
22

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?

28

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;.)

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
1206–1210

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.Fri, Mar 19, 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.)