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

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

It was very good to separate this out, thanks.  Can you can do some TMP performance testing, to verify the impacts are negligible before taking resugaring into consideration, to allay potential concerns?



================
Comment at: clang/include/clang/AST/Type.h:1838
     /// metaprogramming we'd prefer to keep it as large as possible.
-    /// At the moment it has been left as a non-bitfield since this type
-    /// safely fits in 64 bits as an unsigned, so there is no reason to
-    /// introduce the performance impact of a bitfield.
-    unsigned NumArgs;
+    unsigned NumArgs : 16;
   };
----------------
I can't imagine that limiting template arg index to 16 bits from 32 could be all that limiting, but given the comment in the original have you tested/confirmed that this is acceptable?


================
Comment at: clang/include/clang/AST/Type.h:5010
 
-  SubstTemplateTypeParmType(const TemplateTypeParmType *Param, QualType Canon,
-                            Optional<unsigned> PackIndex)
-      : Type(SubstTemplateTypeParm, Canon, Canon->getDependence()),
-        Replaced(Param) {
-    SubstTemplateTypeParmTypeBits.PackIndex = PackIndex ? *PackIndex + 1 : 0;
-  }
+  // The templated entity which owned the substituted type parameter.
+  Decl *ReplacedDecl;
----------------
This description is inconsistent with the `getReplacedDecl()` documentation: this one says its templated, the other says its a template specialization.  I think it is a template or templated decl, correct?  In either case, I think you can remove this comment and just be sure `getReplacedDecl()` is documented accurately.

Also, I wonder if there is a better name than "ReplacedDecl", since that name suggests it should return a `TemplateTypeParmDecl` or similar, when its really the template-parameterized declaration that holds the parameter this replaces.  Maybe "ReplacedParent"?  Or "ReplacedParmParent"?


================
Comment at: clang/include/clang/AST/Type.h:5075
 
-  SubstTemplateTypeParmPackType(const TemplateTypeParmType *Param,
-                                QualType Canon,
+  // The template specialization which owned the substituted type parameter.
+  Decl *ReplacedDecl;
----------------
See above


================
Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1061
       return false;
+    if (Subst1->getReplacedDecl() != Subst2->getReplacedDecl())
+      return false;
----------------
Should this line instead be something like `if !IsStructurallyEquivalent(*this, Subst1->getReplacedDecl(), Subst2->getReplacedDecl())`?


================
Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1073
     const auto *Subst2 = cast<SubstTemplateTypeParmPackType>(T2);
-    if (!IsStructurallyEquivalent(Context,
-                                  QualType(Subst1->getReplacedParameter(), 0),
-                                  QualType(Subst2->getReplacedParameter(), 0)))
+    if (Subst1->getReplacedDecl() != Subst2->getReplacedDecl())
+      return false;
----------------
See above


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1787-1788
   const TemplateTypeParmType *T = TL.getTypePtr();
+  TemplateTypeParmDecl *NewTTPDecl = cast_or_null<TemplateTypeParmDecl>(
+      TransformDecl(TL.getNameLoc(), T->getDecl()));
+
----------------
I don't think this needs to have been moved up here from line 1855; move it back down so the comment down there still makes sense


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858