[libcxx-commits] [PATCH] D131858: [clang] Track the templated entity in type substitution.

[David Rector via Phabricator via libcxx-commits]

davrec added a subscriber: sammccall.
davrec added a comment.

My concerns have been addressed, except for some more nitpicking on names which I think are uncontroversial.

Regarding the modules breakage found by @ChuanqiXu:

> Well we touch FunctionTemplates and VariableTemplates in this patch, because they were not doing template first.
> For whatever reason, class templates were already doing template first, so no need to fix that.
>
> So this patch at least puts that into consistency.

Is there already an analogous breakage for classes then?

Given @mizvekov's approaching deadline and @ChuanqiXu's limited availability, I'm inclined to think if @mizvekov can hack together a reasonable fix for this breakage, leaving a more rigorous/general solution for later as @ChuanqiXu suggests he would like to pursue, that would be sufficient to land this.  Does anyone disagree?



================
Comment at: clang/include/clang/AST/ASTContext.h:1621-1623
+                                        Decl *ReplacedDecl, unsigned Index,
                                         Optional<unsigned> PackIndex) const;
+  QualType getSubstTemplateTypeParmPackType(Decl *ReplacedDecl, unsigned Index,
----------------
AssociatedDecl


================
Comment at: clang/include/clang/AST/Type.h:5060
+  /// Gets the template parameter declaration that was substituted for.
+  const TemplateTypeParmDecl *getReplacedParameter() const;
+
----------------
@sammccall , drafting you as a representative for downstream AST users, do you have any objection to changing this return type to a `TemplateTypeParmDecl` from a `TemplateTypeParmType`?  IIUC the change is not strictly necessary but is more for convenience, correct me if I'm wrong @mizvekov.  But I'm inclined to think STTPT is not much used downstream so the churn will be minimal/acceptable, am I wrong?


================
Comment at: clang/include/clang/AST/TypeProperties.td:730
   }
+  def : Property<"replacedDecl", DeclRef> {
+    let Read = [{ node->getAssociatedDecl() }];
----------------
"associatedDecl"


================
Comment at: clang/include/clang/AST/TypeProperties.td:743
     return ctx.getSubstTemplateTypeParmType(
-        cast<TemplateTypeParmType>(replacedParameter),
-        replacementType, PackIndex);
+        replacementType, replacedDecl, Index, PackIndex);
   }]>;
----------------
associatedDecl


================
Comment at: clang/include/clang/AST/TypeProperties.td:762
 let Class = SubstTemplateTypeParmPackType in {
-  def : Property<"replacedParameter", QualType> {
-    let Read = [{ QualType(node->getReplacedParameter(), 0) }];
+  def : Property<"replacedDecl", DeclRef> {
+    let Read = [{ node->getAssociatedDecl() }];
----------------
"associatedDecl"


================
Comment at: clang/include/clang/AST/TypeProperties.td:774
     return ctx.getSubstTemplateTypeParmPackType(
-                         cast<TemplateTypeParmType>(replacedParameter),
-                        replacementPack);
+                        replacedDecl, Index, replacementPack);
   }]>;
----------------
associatedDecl


================
Comment at: clang/include/clang/Sema/Template.h:80
+    struct ArgumentListLevel {
+      Decl *AssociatedDecl;
+      ArgList Args;
----------------
Actually I think this one should be changed back to `ReplacedDecl` :)
ReplacedDecl makes perfect sense in MLTAL, AssociatedDecl seems to make better sense in STTPT.


================
Comment at: clang/include/clang/Sema/Template.h:163
+    /// cases might even be a template parameter itself.
+    Decl *getAssociatedDecl(unsigned Depth) const {
+      assert(NumRetainedOuterLevels <= Depth && Depth < getNumLevels());
----------------
getReplacedDecl


================
Comment at: clang/include/clang/Sema/Template.h:205
     /// list.
-    void addOuterTemplateArguments(const TemplateArgumentList *TemplateArgs) {
-      addOuterTemplateArguments(ArgList(TemplateArgs->data(),
-                                        TemplateArgs->size()));
+    void addOuterTemplateArguments(Decl *AssociatedDecl, ArgList Args) {
+      assert(!NumRetainedOuterLevels &&
----------------
ReplacedDecl


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131858/new/

https://reviews.llvm.org/D131858