[PATCH] D84048: DR2303: Prefer 'nearer' base classes during template deduction.

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

rsmith added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1204-1205
 
+// Attempt to deduce the template arguments by checking the base types according
+// to (C++ [temp.deduct.call] p4b3.
+///
----------------
Missing `///`


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1281
+             "Base class that isn't a record?");
+      ToVisit.push_back(Base.getType()->getAs<RecordType>());
+    }
----------------
It would be better to add the class to `Visited` here rather than in the loop below -- that is, only add each class to `ToVisit` once rather than only processing each class once. That would put a tighter upper bound on the size of `ToVisit`.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1329-1331
+      Sema::TemplateDeductionResult BaseResult = DeduceTemplateArguments(
+          S, TemplateParams, SpecParam, QualType(NextT, 0), CurMatch.BaseInfo,
+          CurMatch.Deduction);
----------------
This deduction step seems unnecessary to me (whether deduction succeeds or not here has no impact on the result of the algorithm).

Instead, you could perform the `erase_if` call below unconditionally. In order for that to be efficient, it'd make sense to also convert `Matches` into a hash map from (canonical) `CXXRecordDecl*` to `BaseMatch`.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1333
+
+      // If this iS a match, it isn't valid due to CWG2303. So, remove it
+      // from the possible matches.
----------------
Typo "iS".


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1351-1353
+  Info.Param = Matches[0].BaseInfo.Param;
+  Info.FirstArg = Matches[0].BaseInfo.FirstArg;
+  Info.SecondArg = Matches[0].BaseInfo.SecondArg;
----------------
These fields are used to determine how to diagnose a deduction failure, and don't mean anything if deduction succeeds. I think this (and the tracking of `BaseInfo` above) is all dead code (and the corresponding code was similarly dead prior to this change).


================
Comment at: clang/test/CXX/drs/dr23xx.cpp:118
+#if __cplusplus >= 201103L
+namespace dr2303 {
+template <typename... T>
----------------
erichkeane wrote:
> rsmith wrote:
> > This should include a comment that `make_cxx_dr_status` can parse, such as `// dr2303: 11` to indicate support in Clang 11 onwards.
> Our current clang-version is 12.0.0, so 12 is correct here, right?
> 
> I've not been able to get make_cxx_dr_status work unfortunately. It seems to generate a blank version of the file (well, it HAS html, but none of the content).
> 
> I used the cwg_index.html from here: http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_index.html
> 
> 
Oh, right, version 11 already forked. How time flies =) Yes, 12 is correct.

The current list is built from revision 101m of the core issues list. You'll need to grab that from the WG21 wiki; there hasn't been a public release of the core issues list in over 2 years.  (Though it looks like you got this working anyway?)


================
Comment at: clang/www/cxx_dr_status.html:1507
     <td>Destructor lookup</td>
-    <td class="unreleased" align="center">Clang 11</td>
+    <td class="full" align="center">Clang 11</td>
   </tr>
----------------
Please commit the update from "unreleased" to "full" for Clang 11 changes separately.


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

https://reviews.llvm.org/D84048