[PATCH] D87563: [Sema] Improve overload resolution

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Looks like this is an implementation of DR1307. Please also add a test to `test/CXX/drs/dr13xx.cpp`.



================
Comment at: clang/include/clang/Sema/Overload.h:548-549
+
+    /// The std::initializer_list expression to convert from.
+    const InitListExpr *StandardInitializerListFrom{nullptr};
+
----------------
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.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:3690-3703
+class CompareListInitializationSequences {
+  // List-initialization sequence L1 is a better conversion sequence than
+  // list-initialization sequence L2 if:
+  // ...
+  // - C++17:
+  //   L1 converts to type "array of N1 T", L2 converts to type "array of N2 T",
+  //   and N1 is smaller than N2.,
----------------
I think this would be simpler if structured a little differently:

* Rename `CompareListInitializationSequences::Conversion` to `ListInitializationSequence`.
* Make `CompareListInitializationSequences::Compare` a static member of `ListInitializationSequence`.
* Move the element type comparison into `...::Compare` and remove the `CompareListInitializationSequences` class, inlining the (now very simple) constructor into its only user.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:3856-3861
     if (ICS1.isStdInitializerListElement() &&
         !ICS2.isStdInitializerListElement())
       return ImplicitConversionSequence::Better;
     if (!ICS1.isStdInitializerListElement() &&
         ICS2.isStdInitializerListElement())
       return ImplicitConversionSequence::Worse;
----------------
Should this be part of `CompareListInitializationSequences` too?


================
Comment at: clang/lib/Sema/SemaOverload.cpp:5150-5156
+    } else if (ToType->isConstantArrayType()) {
+      // Has the initializer list exactly N elements or fewer than N elements?
+      uint64_t Size = S.getASTContext()
+                          .getAsConstantArrayType(ToType)
+                          ->getSize()
+                          .getZExtValue();
+      if (From->getNumInits() > Size)
----------------
Simplify, and we may as well avoid assuming that the size fits in 64 bits since it's easy to do so here (even though there are lots of other places where we make that assumption).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87563