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

Refactor internal error handling #1952

Merged
merged 2 commits into from Aug 11, 2022
Merged

Refactor internal error handling #1952

merged 2 commits into from Aug 11, 2022

Conversation

Rua
Copy link
Contributor

@Rua Rua commented Aug 11, 2022

This renames the internal Error type to VulkanError (so that it no longer conflicts with std::error::Error) and auto-generates it from vk.xml. The check_errors function and the Success enum are also removed. There is also a new impl From<ash::vk::Result> for VulkanError, which is tolerant to error codes it doesn't know about; it will wrap them in the Unnamed variant, whereas check_errors would panic before.

My intention for the future is to return VulkanError directly to the user. Currently, there are From<VulkanError> implementations for every error type, which translate known errors for that function. But this adds a lot of duplication, especially for each Display implementation that has to repeat the same error messages for standard Vulkan errors.

Moreover, these From<VulkanError> implementations currently panic if a Vulkan API function returns an error that is not expected for that function. Some Vulkan implementations will return errors that aren't in the spec, and Vulkano will therefore indiscriminately crash the user's program based on something they have no control over, which is not very helpful. So instead of trying to translate each known error and panicking on the remainder, returning VulkanError directly would be more tolerant of unusual Vulkan implementations, letting the user deal with the unexpected behaviour instead.

Copy link
Member

@AustinJ235 AustinJ235 left a comment

Choose a reason for hiding this comment

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

Awesome!

@AustinJ235 AustinJ235 merged commit 604c154 into vulkano-rs:master Aug 11, 2022
@Rua Rua mentioned this pull request Aug 12, 2022
@Rua Rua deleted the error branch September 17, 2022 11:54
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.

None yet

2 participants