sammccall added a comment.
Nit on the commit message: I think this is TemplateArgumentLoc 48 -> 32 bytes,
and DynTypedNode 56 -> 40 bytes.
================
Comment at: clang/include/clang/AST/TemplateBase.h:415
-public:
- constexpr TemplateArgumentLocInfo() : Template({nullptr, nullptr, 0, 0}) {}
+ llvm::PointerIntPair<void *, /*Bits=*/2, Kind> PointerAndKind;
----------------
Can you use PointerUnion<Expr*, T*, TypeSourceInfo*>, and save even more of the
boilerplate?
================
Comment at: clang/include/clang/AST/TemplateBase.h:429
+ auto *T = getTemplate();
+ T->Ctx->Deallocate(T);
+ }
----------------
this is a no-op, and thus not worth stashing a pointer to Ctx for!
It also doesn't delete T, and it's probably best to do that even if it's
(currently) a no-op
================
Comment at: clang/include/clang/AST/TemplateBase.h:429
+ auto *T = getTemplate();
+ T->Ctx->Deallocate(T);
+ }
----------------
sammccall wrote:
> this is a no-op, and thus not worth stashing a pointer to Ctx for!
>
> It also doesn't delete T, and it's probably best to do that even if it's
> (currently) a no-op
If you're going to destroy T in the destructor, then you can't have trivial
copies, as the second one is pointing at deallocated memory.
(Well, apart from the fact that deallocation does nothing).
So I think we probably either want:
- allocation on ASTContext, trivial copies, no deallocation
- allocation on heap, copies reallocate
- allocation on heap using shared_ptr
- copies disallowed (but I think we rely on them being available)
================
Comment at: clang/include/clang/AST/TemplateBase.h:443
SourceLocation EllipsisLoc) {
- Template.Qualifier = QualifierLoc.getNestedNameSpecifier();
- Template.QualifierLocData = QualifierLoc.getOpaqueData();
- Template.TemplateNameLoc = TemplateNameLoc.getRawEncoding();
- Template.EllipsisLoc = EllipsisLoc.getRawEncoding();
+ T *Template = Ctx.Allocate<T>();
+ Template->Qualifier = QualifierLoc.getNestedNameSpecifier();
----------------
this is not "heap allocation" in the usual sense - allocating in the context's
slab allocator is cheaper but can't be deallocated.
Not sure how this compares to simply `new T` instead.
If not, then as with delete, I'd prefer `new (Ctx) T` (I think it's correct
without as long as T is trivial, but less obvious)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87080/new/
https://reviews.llvm.org/D87080
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
