[PATCH] D88220: [C++20] Implement more implicit moves for return statements(Part of P1825R0)

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.)



================
Comment at: clang/lib/Sema/SemaStmt.cpp:3061
+  if (VD->getType().isVolatileQualified())
+    return false;
+
----------------
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.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3085
 ///
-/// This routine implements C++14 [class.copy]p32, which attempts to treat
-/// returned lvalues as rvalues in certain cases (to prefer move construction),
-/// then falls back to treating them as lvalues if that failed.
+/// This routine implements C++20 [class.copy.elision]p3:, which attempts to
+/// treat returned lvalues as rvalues in certain cases (to prefer move
----------------
You changed "p32" to "p3:" — I think you meant "p3" without the colon.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3190
+///           not need to be an rvalue reference to the returned object's type.
+///   C++20
+
----------------
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?


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3210
 
+    bool AffectedByCWG1579 = false;
     if (!NRVOCandidate) {
----------------
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.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3291
   // (again) now with the return value expression as written.
+  // TODO: C++20 considering the expression or operand as an lvalue
   if (Res.isInvalid())
----------------
FWIW, I don't understand this TODO comment. What's to do, here?


================
Comment at: clang/test/SemaCXX/implicit-move.cpp:21
+  NoNeedRvalueRef(const NoNeedRvalueRef &);
+  NoNeedRvalueRef(NoNeedRvalueRef &&);
+  NoNeedRvalueRef(Error);
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88220/new/

https://reviews.llvm.org/D88220