aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:1541
+def ArmSveVectorBits128 : TypeAttr {
+ let Spellings = [];
----------------
c-rhodes wrote:
> sdesmalen wrote:
> > aaron.ballman wrote:
> > > sdesmalen wrote:
> > > > nit: Can you add a comment saying why these are undocumented (and have
> > > > no spellings)
> > > Also, I think these are all missing `let SemaHandler = 0;` and `let
> > > ASTNode = 0;`
> > >
> > > Is there a reason why we need N different type attributes instead of
> > > having a single type attribute which encodes the N as an argument? I
> > > think this may simplify the patch somewhat as you no longer need to
> > > switch over N as much.
> > > Is there a reason why we need N different type attributes instead of
> > > having a single type attribute which encodes the N as an argument?
> > AIUI this was a workaround for getting the value of N from an
> > AttributedType, because this only has `getAttrKind` to return the attribute
> > kind, but no way to get the corresponding argument/value. This seemed like
> > a simple way to do that without having to create a new subclass for Type
> > and having to support that in various places. Is the latter the approach
> > you were thinking of? (or is there perhaps a simpler way?)
> > Also, I think these are all missing let SemaHandler = 0; and let ASTNode =
> > 0;
>
> Good to know. In SemaType I'm doing `CurType = State.getAttributedType(A,
> CurType, CurType);` which gives an `AttributedType` in the AST, should I
> still set `let ASTNode = 0;` in this case?
>
> > Is there a reason why we need N different type attributes instead of having
> > a single type attribute which encodes the N as an argument?
>
> As Sander mentioned, it seemed like the easiest solution, interested to know
> if there's a better approach however
I was thinking specifically of creating a new `Type` subclass and supporting it
rather than adding 5 new attributes that only vary by a bit-width (which means
there's likely to be a 6th someday). It's not immediately clear to me whether
that's a really big ask for little gain or not, though.
================
Comment at: clang/include/clang/Basic/Attr.td:1541
+def ArmSveVectorBits128 : TypeAttr {
+ let Spellings = [];
----------------
aaron.ballman wrote:
> c-rhodes wrote:
> > sdesmalen wrote:
> > > aaron.ballman wrote:
> > > > sdesmalen wrote:
> > > > > nit: Can you add a comment saying why these are undocumented (and
> > > > > have no spellings)
> > > > Also, I think these are all missing `let SemaHandler = 0;` and `let
> > > > ASTNode = 0;`
> > > >
> > > > Is there a reason why we need N different type attributes instead of
> > > > having a single type attribute which encodes the N as an argument? I
> > > > think this may simplify the patch somewhat as you no longer need to
> > > > switch over N as much.
> > > > Is there a reason why we need N different type attributes instead of
> > > > having a single type attribute which encodes the N as an argument?
> > > AIUI this was a workaround for getting the value of N from an
> > > AttributedType, because this only has `getAttrKind` to return the
> > > attribute kind, but no way to get the corresponding argument/value. This
> > > seemed like a simple way to do that without having to create a new
> > > subclass for Type and having to support that in various places. Is the
> > > latter the approach you were thinking of? (or is there perhaps a simpler
> > > way?)
> > > Also, I think these are all missing let SemaHandler = 0; and let ASTNode
> > > = 0;
> >
> > Good to know. In SemaType I'm doing `CurType = State.getAttributedType(A,
> > CurType, CurType);` which gives an `AttributedType` in the AST, should I
> > still set `let ASTNode = 0;` in this case?
> >
> > > Is there a reason why we need N different type attributes instead of
> > > having a single type attribute which encodes the N as an argument?
> >
> > As Sander mentioned, it seemed like the easiest solution, interested to
> > know if there's a better approach however
> I was thinking specifically of creating a new `Type` subclass and supporting
> it rather than adding 5 new attributes that only vary by a bit-width (which
> means there's likely to be a 6th someday). It's not immediately clear to me
> whether that's a really big ask for little gain or not, though.
Ah, you're right, we may still need `ASTNode` to be kept around for that, good
call.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83551/new/
https://reviews.llvm.org/D83551
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
