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

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

Quuxplusone accepted this revision.
Quuxplusone added a comment.
This revision is now accepted and ready to land.

I don't fully understand the new control flow, but at least the new //behavior// (after applying this patch) looks like an improvement to me.
I recommend rebasing on top-of-tree, mainly so that the buildbots will run again and presumably be greener this time.

I still doubt that I have the authority to "accept" this patch, and am hoping to see someone like @zygoloid weigh in, even if it's only to say "meh." I //am// willing and able to //physically// land this patch, if someone says it's appropriate for me to do so.



================
Comment at: clang/lib/Sema/SemaInit.cpp:4181
+  // For deleted functions in other contexts, there is no need to get the
+  // initialization sequence.
+  if (Result == OR_Deleted && Kind.getKind() != InitializationKind::IK_Copy)
----------------
IIUC, this comment is explaining the motivation for repeating `if (Result == OR_Deleted) // don't return quite yet` twice in the code above — line 4123 and line 4146. It might be better to move the comment higher, then.


================
Comment at: clang/lib/Sema/SemaInit.cpp:5285
+    // need the initialization sequence to decide whether to perform the second
+    // overload resolution.
+    if (!(Result == OR_Deleted &&
----------------
Checking my understanding: If the first resolution selects a deleted function which is a constructor whose first parameter is an rvalue reference to T, then we don't perform the second resolution. If the first resolution selects a deleted function which is not a constructor, or whose parameter is of the wrong type, then (in C++11 through C++17 but not in C++20) we //do// perform the second resolution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936