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

Tue Feb 14 07:17:28 PST 2023

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