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

Tue Jan 5 07:20:16 PST 2021

Quuxplusone added inline comments.

================
Comment at: clang/lib/Sema/SemaInit.cpp:4020
                                          bool IsListInit = false,
                                          bool IsInitListCopy = false) {
   assert(((!IsListInit && !IsInitListCopy) ||
----------------
Two comments you could act on if you want, or just ignore if they seem too scope-creepy:

(1) This function is //crazy// long and complicated. It would be great to break it down into named steps or subtasks.

(2) I'm still concerned about the repetition of `if (Result == OR_Deleted but Kind == IK_Copy) keep going`. IIUC, the intent is that this function should return the `InitializationSequence` that is found by overload resolution... but sometimes we can tell that overload resolution is going to find an inappropriate sequence (e.g. an explicit conversion, or a sequence that depends on a deleted function, or maybe some other situations) and in that case we want to short-circuit, because the caller is going to treat "No unique sequence" in the same way as "The unique sequence was inappropriate".  **Would it be better to** have the caller pass in a new function parameter, `bool ShortCircuitUnusableSequences`, which is usually `true` but can be explicitly set to `false` in the three cases (`return`, `co_return`, `throw`) where we currently want to treat "No unique sequence" differently from "The unique sequence was inappropriate"?

Then the repeated check would be `if (Result == OR_Deleted && ShortCircuitUnusableSequences) return;` and it would be a little clearer what's going on.

(Sidenote: Default function arguments, //especially// trailing boolean ones, [are the devil](https://quuxplusone.github.io/blog/2020/04/18/default-function-arguments-are-the-devil/#the-boolean-parameter-tarpit).)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936