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