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

Fix some clippy errors #235

Merged
merged 4 commits into from
Jul 8, 2022
Merged

Fix some clippy errors #235

merged 4 commits into from
Jul 8, 2022

Conversation

p3t3rix
Copy link
Contributor

@p3t3rix p3t3rix commented Jul 7, 2022

Wanted to fix some clippy errors to make CI/CD green again.
The clippy allow was added because extensions turning official still remain in the grammar.json and generate arms with the same opcode. Not sure if this should be fixed in another way but i think making CI green should be enough for the near future.
I just touched clippy errors that were thrown by ci/cd when it last ran (for example here https://github.com/gfx-rs/rspirv/runs/6061904152?check_suite_focus=true).

@MarijnS95
Copy link
Collaborator

The clippy allow was added because extensions turning official still remain in the grammar.json and generate arms with the same opcode. Not sure if this should be fixed in another way but i think making CI green should be enough for the near future.

I don't think we should hide these, but find a solution instead. It's not only the match arm but the duplicate enum variants may also be confusing/wrong. Rather have red CI long enough so that someone picks them up and fixes this, may actually be pretty trivial.

@p3t3rix
Copy link
Contributor Author

p3t3rix commented Jul 7, 2022

In this case i will close this PR and open an issue for someone else to fix because i don't have the experience to fix the issue itself, if that's ok with you.

@MarijnS95
Copy link
Collaborator

@p3t3rix There are still two other issues that this PR fixes, you can leave it open for those and remove the #[allow(clippy::match_overlapping_arm)], then make an issue to address that separately.

…mmands in the spirv grammar e.g. KHR extensions turning offical"

This reverts commit 44e540f.
@p3t3rix
Copy link
Contributor Author

p3t3rix commented Jul 7, 2022

Reverted the clippy allow and created an issue for the duplicated op-codes: #236

Copy link
Collaborator

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

Thanks!

@msiglreith msiglreith merged commit aca957a into gfx-rs:master Jul 8, 2022
@p3t3rix p3t3rix deleted the fixclippy branch July 8, 2022 10:56
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.

3 participants