[PATCH] D87563: [Sema] Improve overload resolution

[Mark de Wever via Phabricator via cfe-commits]

Mordante added inline comments.


================
Comment at: clang/include/clang/Sema/Overload.h:548-549
+
+    /// The std::initializer_list expression to convert from.
+    const InitListExpr *StandardInitializerListFrom{nullptr};
+
----------------
rsmith wrote:
> Storing the `IntListExpr` here doesn't seem like it captures the necessary information. Consider:
> 
> ```
> void f2(std::pair<const char*, const char*> (&&)[]); // #1
> void f2(std::string (&&)[]); // #2
> void g2() { f2({"foo","bar"}); } // should call #2
> ```
> 
> Here, the `InitListExpr` in both cases has two elements. We don't store the converted `InitListExpr`, because we don't -- and aren't allowed to -- build the converted `InitListExpr` until we've chosen the right overload. But I think what we're supposed to do in this case is to notice that calling #1 would initialize only one element of the array (one pair), whereas calling #2 would initialize two elements of the array, so we should call #2 here.
> 
> In effect, what you need to do is to compute the effective array bound for the case where we're initializing an array of unknown bound, and prefer the conversion sequence with the lower effective array bound. `InitListChecker` already works this out internally, see [here](https://github.com/llvm/llvm-project/blob/c0bcd11068fc13e45b253c6c315882097f94c121/clang/lib/Sema/SemaInit.cpp#L1937), but currently only does that when actually doing the conversion (when not in `VerifyOnly` mode); you'd need to adjust that.
> 
> Given that the code that needs this (the P0388 part) is dead code for now, perhaps the best thing would be to do only the array bound comparison in this patch, and we can compute the proper array bound for said comparison in a future patch.
I've removed the unused code for P0338 for now. Instead of storing the `InitListExpr` I've stored the size difference between the array and the initializer list.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87563