-
Notifications
You must be signed in to change notification settings - Fork 257
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
Enforce grammar naming conventions for aliases #432
Enforce grammar naming conventions for aliases #432
Conversation
Fixes KhronosGroup#429 * Enforce Core -> KHR -> EXT -> Vendor conventions for aliased names * Update grammar to satisfy these conventions and regenerate headers
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.
LGTM. This will cause disassembly to change. So it may require SPIRV-Tools 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.
Thanks for doing this!
@@ -5938,7 +5938,7 @@ | |||
"version" : "None" | |||
}, | |||
{ | |||
"opname" : "OpReportIntersectionKHR", | |||
"opname" : "OpReportIntersectionNV", |
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.
Not for this change but this makes me wonder whether we shouldn't introduce a way of encoding aliases that does not require duplicating whole definitions. The first idea that comes to mind is something like:
{
"opname": "OpReportIntersectionKHR",
"aliases": ["OpReportIntersectionNV"]
}
Is there a reason we wouldn't want to do something like this?
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 the topic of #138
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.
My opinion is that we should do something along those lines, but it will take longer to nail down how we want it to work (e.g. in conjunction with the spec). So for now this is desirable.
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.
Yeah, it's a much bigger change. I'm not suggesting we try to do this here and now, just checking that I hadn't missed a reason we couldn't (or didn't want to) go there.
Merging as discussed in the June 5th teleconference. |
Fixes #429