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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
