tahonermann accepted this revision. tahonermann added a comment. This revision is now accepted and ready to land.
I think this looks good. I took more time to compare with the current code and it looks to me like behavioral consistency is maintained where desired. I added one comments requesting a change to a comment (to be made when committing the changes) and another comment with a question asked to improve my own understanding (that might also indicate that some additional comments would be helpful). ================ Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:75-77 +// Add template arguments from a variable template instantiation. For a +// class-scope explicit specialization, there are no template arguments +// at this level, but there may be enclosing template arguments. ---------------- I think the selected portion of this comment would be better placed inside the function before the call to `isClassScopeExplicitSpecialization`. ================ Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:255-258 +/// \param ForConstraintInstantiation when collecting arguments, +/// ForConstraintInstantiation indicates we should continue looking when +/// encountering a lambda generic call operator, and continue looking for +/// arguments on an enclosing class template. ---------------- More a question than a review comment: why is `ForConstraintInstantiation` needed? The behavior it enables seems to me like the behavior that would always be desired. When would template arguments for such enclosing templates not be wanted? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134874/new/ https://reviews.llvm.org/D134874 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
