aaron.ballman added a reviewer: erichkeane. aaron.ballman added a comment. Adding Erich as he's been staring at the template instantiation code recently and may have thoughts/opinions. To me, this seems like the correct approach though.
================ Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2536 - TypeSourceInfo *BaseTypeLoc = SubstType(Base.getTypeSourceInfo(), - TemplateArgs, - Base.getSourceRange().getBegin(), - DeclarationName()); + BaseTypeLoc = + SubstType(Base.getTypeSourceInfo(), TemplateArgs, ---------------- RKSimon wrote: > I'm not familiar with this code at all - but we've gone from having a local > shadow BaseTypeLoc variable to reusing the BaseTypeLoc declared at line#2510 > - is that what we actually want? I don't think it matters. After this `for` loop terminates, we `continue` the outer loop. That said, this does make the code harder to read, we may want to use a local instead of reusing the outer variable like this. ================ Comment at: clang/test/SemaCXX/template-base-class-pack-expansion.cpp:2 +// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s +// Don't crash (#53609). + ---------------- Can we test more than just not crashing? (e.g., can we test that the expansion happened properly?) Perhaps an AST dump test, at a minimum? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119063/new/ https://reviews.llvm.org/D119063 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
