Skip to content
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

sr: Deduplicate match ranges like spirv header #242

Merged

Conversation

MarijnS95
Copy link
Collaborator

Solve some clippy lints by getting rid of these aliases of overlapping Ops. Unfortunately, just like in spirv, this biases towards the NV rather than KHR stabilized raytracing ops.

Note that in spirv the Ops are aliases in code rather than omitted altogether: don't think we can do that for the range match here though unless deducing whether the shader was compiled with the NV or KHR variant, even though the KHR variant is already stabilized for quite some time.

Solve some clippy lints by getting rid of these aliases of overlapping
`Op`s.  Unfortunately, just like in `spirv`, this biases towards the
`NV` rather than `KHR` stabilized raytracing ops.

Note that in `spirv` the `Op`s are aliases in code rather than omitted
altogether: don't think we can do that for the range match here though
unless deducing whether the shader was compiled with the NV or KHR
variant, even though the KHR variant is already stabilized for quite
some time.
@MarijnS95 MarijnS95 force-pushed the sr-remove-overlapping-match-ranges branch from d29152c to f19da5f Compare April 6, 2023 17:28
@MarijnS95
Copy link
Collaborator Author

@msiglreith are you okay with this change, or should we see if we can prefer the KHR types here (and maybe also in the spirv crate)?

@msiglreith
Copy link
Contributor

@MarijnS95 KHR > EXT > Vendor specific would be great but the current approach seems also totally fine to me

@MarijnS95
Copy link
Collaborator Author

@msiglreith definitely, it might just be a hassle to implement as all parts of the generator redo these kinds of evaluations on the parsed SPIR-V grammar :(

@MarijnS95
Copy link
Collaborator Author

@msiglreith done now!

@MarijnS95 MarijnS95 force-pushed the sr-remove-overlapping-match-ranges branch from 13892e1 to f0c0d46 Compare April 6, 2023 18:42
@MarijnS95 MarijnS95 force-pushed the sr-remove-overlapping-match-ranges branch from f0c0d46 to 05d13a1 Compare April 6, 2023 18:42
@MarijnS95 MarijnS95 requested a review from msiglreith April 6, 2023 19:10
Copy link
Contributor

@msiglreith msiglreith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great thanks!
(huh so many vendors already)

@msiglreith msiglreith merged commit aa260e0 into gfx-rs:master Apr 6, 2023
@MarijnS95 MarijnS95 deleted the sr-remove-overlapping-match-ranges branch April 6, 2023 20:15
@MarijnS95
Copy link
Collaborator Author

(huh so many vendors already)

Yup rather than only listing the few vendors with ops currently I just copied the whole thing from ash-rs/ash#676.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants