[PATCH] D87080: [AST] Reduce the size of TemplateArgumentLocInfo.

[Sam McCall via Phabricator via cfe-commits]

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