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

Remove unused attributes from storage class #807

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Remove unused attributes from storage class #807

wants to merge 2 commits into from

Conversation

andrewleverette
Copy link

resolves #568

Copy link
Contributor

@khyperia khyperia left a comment

Choose a reason for hiding this comment

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

I believe a few error messages and tests need to be updated with the removal of uniform_constant

@andrewleverette
Copy link
Author

Sorry for the late reply, I'm just now catching up after the holiday. I ran the test suite before I submitted the pull request, but I've only just realized the CI test stage is running tests differently. I'm new to the project so I'm not sure what needs to be changed. If you could clarify or provide some context for the necessary changes, I would definitely appreciate it. I'm still happy to work on this task.

@expenses
Copy link
Contributor

Sorry for the late reply, I'm just now catching up after the holiday. I ran the test suite before I submitted the pull request, but I've only just realized the CI test stage is running tests differently. I'm new to the project so I'm not sure what needs to be changed. If you could clarify or provide some context for the necessary changes, I would definitely appreciate it. I'm still happy to work on this task.

As you removed all the attributes that are used in the test: https://github.com/EmbarkStudios/rust-gpu/blob/main/tests/ui/spirv-attr/invalid-storage-class.rs, the failure message of the test is now different.

You could either run the test command cargo run -p compiletests --release --no-default-features --features "use-installed-tools" -- --target-env vulkan1.1,vulkan1.2,spv1.3 with the --bless flag, or probably just remove the test as it doesn't make sense anymore.

@andrewleverette
Copy link
Author

I removed the test case that you mentioned. I did run the command with the --bless' to retest but it looks like it changed several files. Let me know if this should be reverted and just delete the test case.

@@ -1,7 +1,14 @@
error: error:0:0 - failed spawn executable: No such file or directory (os error 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want this error to be here :P Do you have spirv-val installed and in your $PATH?

@khyperia
Copy link
Contributor

Additionally, the errors themselves needs to be updated, as they can no longer happen.

@eddyb eddyb added the s: waiting on review PRs that blocked on a team member's review. label May 21, 2022
@oisyn oisyn added the s: needs update This issue needs a follow up or investigation whether it still applies. label Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: needs update This issue needs a follow up or investigation whether it still applies. s: waiting on review PRs that blocked on a team member's review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove unneeded Storage Class attributes
5 participants