-
Notifications
You must be signed in to change notification settings - Fork 795
[SYCL] Let tablegen handle mutually exclusive decl attrs #3507
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The new MutualExclusions tablegen facilities allow us to remove a bunch of custom diagnostic checking code. This also fixes an issue with the upstream implementation of the MutualExclusions facilities for DeclOrStmtAttr attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
| bool CurAttrIsStmtAttr = | ||
| Attr.isSubClassOf("StmtAttr") || Attr.isSubClassOf("DeclOrStmtAttr"); | ||
| bool CurAttrIsDeclAttr = | ||
| !CurAttrIsStmtAttr || Attr.isSubClassOf("DeclOrStmtAttr"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AaronBallman, isn't the second bool the same as:
!Attr.isSubClassOf("StmtAttr") || Attr.isSubClassOf("DeclOrStmtAttr");
Wouldn't this match anything that is not StmtAttr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct.
There are three kinds of attributes (four-ish if you count pragmas, but those never get here): type, stmt, and decl. We bail out early if it's a type attribute on line 3664, so by this point it should either be a statement attr, a decl attr, or both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you want to handle this PR? Is there a separate PR to address statement attributes? I just don't want us to lose track of it. I will approve this one, assuming that one won't fall through the cracks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a separate PR to address statement attributes?
I think so. Here is the discussion:
Originally posted by @AaronBallman in #3507 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Statement attributes need more love; I'm working on that refactoring currently. If it looks like that refactoring has to be set down for some reason, I'll file an issue about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@premanandrao, @AaronBallman, is this conversation resolved or I should wait for PR updates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not expecting to change this PR further unless I've missed something or there are other concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@premanandrao, @AaronBallman, is this conversation resolved or I should wait for PR updates?
Sorry, I should have been more explicit. I considered this resolved.
The new MutualExclusions tablegen facilities allow us to remove a bunch
of custom diagnostic checking code. This also fixes an issue with the
upstream implementation of the MutualExclusions facilities for
DeclOrStmtAttr attributes.