[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

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

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