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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
