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