erichkeane added inline comments.
================
Comment at: clang/lib/AST/ASTContext.cpp:2154
+ if (uint64_t Result = SubstTypeVisitor(Ctx, SubType->getReplacementType())
+ .TryEval(Attr->getAlignmentExpr())) {
+ TI.Align = Result;
----------------
aaron.ballman wrote:
> rsmith wrote:
> > I don't think this approach is going to work out well. There are many cases
> > this gets wrong, fixing them would require reinventing template
> > substitution here, and generally we shouldn't be doing this substitution as
> > part of alignment computation at all -- we should have `TreeTransform`
> > produce the right alignment value during template instantiation and just
> > pull it back out of the `Type` here. We can't really use the same approach
> > as we do for `TypedefDecl`s, though, because we don't instantiate a
> > `TypeAliasDecl` for each use of a `TypeAliasTemplateDecl`, so there's
> > nowhere to hang an instantiated attribute. But the handling for
> > `TypedefType`s is also fairly painful, so I think we should be considering
> > an approach that makes both of them more elegant. Here's what I suggest:
> >
> > - We add a new sugar-only `Type` subclass that represents an alignment
> > attribute. Maybe we can model this as an `AttributedType` for the
> > `AlignedAttr`, or maybe we create a new kind of type node.
> > - We translate `AlignedAttr`s on typedef and type alias declarations into
> > this new kind of `Type` wrapping the declared type.
> > - We make `getTypeInfoImpl` special-case that type sugar node instead of
> > special-casing `TypedefType` sugar.
> > - We make sure that `TreeTransform` properly instantiates the new node, in
> > particular performing substitution within the argument.
> I think this approach makes sense to me, but it's worth noting: the alignment
> attributes (vendor specific or standard) are declaration attributes and not
> type attributes. I think this is a design mistake; alignment is a fundamental
> property of a type (see http://eel.is/c++draft/basic.align#1), so I'm in
> favor of modeling this as a type attribute, but the standard `alignas`
> attribute does not have type semantics: http://eel.is/c++draft/dcl.align#1.
> Should we approach the standards committees about this or are we not
> concerned about `alignas` behavior?
>
> (If we expect `alignas` to be a declaration attribute and `[[gnu::aligned]]`
> to be a type attribute, another question is whether we should split them in
> Attr.td.)
IMO, the idea of doing this via this visitor is incorrect, we should just make
sure the instantiated decl of the type alias to get the attribute, we do
similar things for function decls.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143533/new/
https://reviews.llvm.org/D143533
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
