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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/include/clang/AST/Expr.h:963
 };
+static_assert(llvm::PointerLikeTypeTraits<Expr *>::SpecializationForExpr,
+              "Specialization in TemplateBase.h must be seen here");
----------------
I think this is it:
```
// PointerLikeTypeTraits is specialized so it can be used with a forward-decl of Expr.
// Verify that we got it right.
static_assert((1 << PointerLikeTypeTraits<Expr*>::NumLowBitsAvailable) == alignof(Expr), "PointerLikeTypeTraits<Expr*> assumes too much alignment");
```

(Or just `% alignof(Expr) == 0` if you want a weaker condition)


================
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; }
----------------
rsmith wrote:
> hokein wrote:
> > sammccall wrote:
> > > 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)
> > yes, exactly. 
> > 
> > do you have a better idea on how the static_assert should look like? The way I can think of is to add a new flag in this specialization, and use `static_assert(PointerLikeTypeTraits<clang::Expr *>::flag, "...")`.
> This may be included from `Expr.h` right now, but really doesn't seem like the right header to hold this specialization. Perhaps we should consider adding something like an `AST/ForwardDecls.h`, containing forward declarations and specializations such as this one, and include that from `Expr.h` and from here?
I also had this thought but wasn't sure. @hokein maybe a trivial followup patch rather than inline here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87080