Page MenuHomePhabricator

[C++20] Implement more implicit moves for return statements(Part of P1825R0)
Needs ReviewPublic

Authored by nullptr.cpp on Thu, Sep 24, 5:18 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

In P1825R0, the first parameter of overload resolution selected function does
not need to be an rvalue reference to the returned object's type.

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

nullptr.cpp created this revision.Thu, Sep 24, 5:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Sep 24, 5:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
nullptr.cpp requested review of this revision.Thu, Sep 24, 5:18 AM

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
3061

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.

3085

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

3190

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?

3210

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.

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.