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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
