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

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 5 inline comments as done.
erichkeane added a comment.

Thanks for the review!  I'll get this updated in the morning.  I DO have a question on your suggestion for the ToVisit/Visited example, so if you could explain a little better, I'd be grateful.



================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1281
+             "Base class that isn't a record?");
+      ToVisit.push_back(Base.getType()->getAs<RecordType>());
+    }
----------------
rsmith wrote:
> 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`.
I'm perhaps missing something here... Can you clarify your suggestion a bit more?  If we add it to 'Visited' here, it will never get visited in the while-loop below, right?  


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1329-1331
+      Sema::TemplateDeductionResult BaseResult = DeduceTemplateArguments(
+          S, TemplateParams, SpecParam, QualType(NextT, 0), CurMatch.BaseInfo,
+          CurMatch.Deduction);
----------------
rsmith wrote:
> 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`.
Ah, yes, thats a really good observation!  I'll do that.


================
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;
----------------
rsmith wrote:
> 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).
I see, so all tracking of BaseInfo isn't useful?  That will simplify the code quite a bit then, since BaseInfo accounts for nearly all the code in BaseMatch (both the move operations, and the constructor only exist because of it).  BaseMatch becomes essentially a pair otherwise. I think I can actually remove BaseMatch entirely as a result, and change the Matches to a map from RecordType* to the SmallVector of Deduction state.


================
Comment at: clang/test/CXX/drs/dr23xx.cpp:118
+#if __cplusplus >= 201103L
+namespace dr2303 {
+template <typename... T>
----------------
rsmith wrote:
> 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?)
Yep, I figured that out with help from @aaron.ballman on IRC :)  I downloaded 101m from the wiki.


================
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>
----------------
rsmith wrote:
> Please commit the update from "unreleased" to "full" for Clang 11 changes separately.
Will do!  I'll do that as review-after-commit, then rebase this on top of it.  Thanks!


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

https://reviews.llvm.org/D84048