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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Looks good apart from a minor technical issue with the traits.

Would it be possible to compile some big file in LLVM (probably doesn't matter much which, Sema something?) and observe if there's a significant change in overall ASTContext size?



================
Comment at: clang/include/clang/AST/TemplateBase.h:43
+// the dependency.
+template <> struct PointerLikeTypeTraits<clang::Expr *> {
+  static inline void *getAsVoidPointer(clang::Expr *P) { return P; }
----------------
At first glance this is unsafe: you could have two different definitions of the same specialization in different files.

In fact it's OK, the default one can now never be instantiated, because Expr.h includes this file and so anyone that can see the definition can see this specialization.

But this is subtle/fragile: at least it needs to be spelled out explicitly in the comment.
I'd also suggest adding a static assert below the definition of Expr that it is compatible with this specialization (because it is sufficiently aligned).

(I can't think of a better alternative - use of PointerUnion is a win, partly *because* it validates the alignment)


================
Comment at: clang/include/clang/AST/TemplateBase.h:431
 
-  TemplateArgumentLocInfo(NestedNameSpecifierLoc QualifierLoc,
+  TemplateArgumentLocInfo(ASTContext &Ctx, NestedNameSpecifierLoc QualifierLoc,
                           SourceLocation TemplateNameLoc,
----------------
the pattern here (use of Ctx for allocation although other variants don't) is unusual, maybe add a comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87080