Quuxplusone added inline comments.
================ Comment at: clang/lib/Sema/SemaStmt.cpp:3110 +/// +/// \returns Whether need to do the second overload resolution. If the first +/// overload resolution fails, or if the first overload resolution success but ---------------- s/need/we need/ ================ Comment at: clang/lib/Sema/SemaStmt.cpp:3111 +/// \returns Whether need to do the second overload resolution. If the first +/// overload resolution fails, or if the first overload resolution success but +/// the selected constructor/operator doesn't match the additional criteria, we ---------------- s/success/succeeds/ ================ Comment at: clang/lib/Sema/SemaStmt.cpp:3212 ExprResult Res = ExprError(); + bool NeedSecondOverload = true; ---------------- Naming nit: I would prefer to call this "NeedSecondStep" or "NeedSecondOverloadResolution" or "NeedSecondResolution". The way it is now, it sounds like it's saying we need to find a second //overload//, which isn't really the intended meaning. ================ Comment at: clang/test/CXX/class/class.init/class.copy.elision/p3.cpp:4 +// RUN: %clang_cc1 -std=c++14 -fsyntax-only -fcxx-exceptions -verify=cxx11_14_17 %s +// RUN: %clang_cc1 -std=c++11 -fsyntax-only -fcxx-exceptions -verify=cxx11_14_17 %s + ---------------- The new behavior looks good to me (once the tests pass). Why are there two different `-verify=` tags here, though? This behavior hasn't changed between C++11/14/17 and C++20. (IIUC, the point of this patch is to fix the implementation divergence described in http://quuxplusone.github.io/draft/d2266-implicit-move-rvalue-ref.html#two-lookups , example `thirteen`.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92936/new/ https://reviews.llvm.org/D92936 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
