hokein 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");
----------------
sammccall wrote:
> 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)
Ah, thanks. `alignof(Expr)` is 8, I feel like comparing the Numbits directly
seems more easy to understand.
================
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; }
----------------
sammccall wrote:
> 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?
that sounds good to me.
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
