aaron.ballman added a reviewer: erichkeane.
aaron.ballman added a comment.
Adding Erich as attributes code owner.
================
Comment at: clang/lib/AST/ASTContext.cpp:2154
+ if (uint64_t Result = SubstTypeVisitor(Ctx, SubType->getReplacementType())
+ .TryEval(Attr->getAlignmentExpr())) {
+ TI.Align = Result;
----------------
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.)
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
