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

[Matheus Izvekov via Phabricator via libcxx-commits]

mizvekov added inline comments.


================
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;
   };
----------------
davrec wrote:
> 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?
Not yet, we will begin performance testing soon. But I am not concerned about this one, as it's easy to get around this limitation by not using the bitfields.


================
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;
----------------
davrec wrote:
> 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"?
Well it's the templated entity that owns this substitution. It unfortunately can't be just a TemplateDecl because some templated entities do not derive from that unfortunately.


================
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()));
+
----------------
davrec wrote:
> 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
Well the comment there does make more sense now, since it was not talking about the block of code that got moved here.

But you are right it's not needed anymore, this is left over code from a previous approach to the problem, before the changes in this patch made it obsolete.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858