[PATCH] D119063: [SemaCXX] Properly scope ArgumentPackSubstitutionIndex when expanding base types

[Aaron Ballman via Phabricator via cfe-commits]

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