[PATCH] D143533: [clang] Allow gnu::aligned attribute to work with templated type alias declarations

Tue Feb 14 06:25:41 PST 2023

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