[RFC] Error handling

[msiglreith]

There are function which convert a successful return code into an error.
For example acquire_next_image may return VK_SUBOPTIMAL_KHR among other codes, indicating that the return index could still be used but the swapchain doesn't match the surface anymore. In the current API, this would return an error.

My suggestion would be to return a tuple of (ReturnType, SuccessCode) in these cases.

[kvark]

Seems a bit inconsistent to return a tuple in some cases and Result in others. And clearly, the latter is preferred if possible. Maybe we can have enumerations for those specific success results, making the return types Result<SpecificSuccessEnum, GenericError> for those cases?

[MaikKlein]

That seems reasonable, I am also leaning more towards custom success enums. Alternatively I think I could also just keep it as it currently is, but return all success error codes in the Ok variant.

pub unsafe fn acquire_next_image_khr(
    &self,
    swapchain: vk::SwapchainKHR,
    timeout: vk::uint64_t,
    semaphore: vk::Semaphore,
    fence: vk::Fence,
) -> VkResult<vk::uint32_t> {
    let mut index = mem::uninitialized();
    let err_code = self.swapchain_fn.acquire_next_image_khr(
        self.handle,
        swapchain,
        timeout,
        semaphore,
        fence,
        &mut index,
    );
    match err_code {
        vk::Result::Success | vk::Resut::SubOptimalKHR | vk::Result::Timeout | .. => Ok(index),
        _ => Err(err_code),
    }
}

Thoughts?

[kvark]

I like that

…

On Dec 30, 2017, at 06:49, Maik Klein ***@***.***> wrote: That seems reasonable, I am also leaning more towards custom success enums. Alternatively I think I could also just keep it as it currently is, but return all success error codes in the Ok variant. pub unsafe fn acquire_next_image_khr( &self, swapchain: vk::SwapchainKHR, timeout: vk::uint64_t, semaphore: vk::Semaphore, fence: vk::Fence, ) -> VkResult<vk::uint32_t> { let mut index = mem::uninitialized(); let err_code = self.swapchain_fn.acquire_next_image_khr( self.handle, swapchain, timeout, semaphore, fence, &mut index, ); match err_code { vk::Result::Success | vk::Resut::SubObtimalKHR | vk::Result::Timeout | .. => Ok(index), _ => Err(err_code), } } Thoughts? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[msiglreith]

Seems a bit inconsistent to return a tuple in some cases and Result in others. And clearly, the latter is preferred if possible.

I agree. I meant more like Result<(Index, SuccessCode), ErrorCode>.
The current form is Result<T, VkResult> for all function. I would suggest here to replace VkResult with ErrorCode containing only negative values (errors) of the VkResult type. If a function returns any other value than Success on success it should be in the Ok variant along the actual return value.

[LPGhatguy]

I like the idea of changing all of the functions to return Result<(T, SuccessCode), ErrorCode> -- I think we should sure not to lose the status code in the case of success.

Is it safe to assume that all success codes are positive and all error codes are negative? If so, that means this change is largely mechanical.

[krolli]

@LPGhatguy Yes, spec guarantees that errors are negative and successes are positive (or 0 in case of VK_SUCCESS).

@MaikKlein I'm leaning more towards the Result<(T, SuccessCode), ErrorCode> version. In your example, consuming success code and returning just the index might not work for all success codes. For example, I think VK_TIMEOUT would not give you valid index, thus consuming status and just returning the index would be an error. Likewise, even if you get valid index, you may want to react to VK_SUBOPTIMAL_KHR and recreate swapchain anyway.

[MaikKlein]

I think Result<(T, SuccessCode), ErrorCode> is not the most consistent choice for ash. What do you think about this?

    pub unsafe fn acquire_next_image_khr(
        &self,
        swapchain: vk::SwapchainKHR,
        timeout: u64,
        semaphore: vk::Semaphore,
        fence: vk::Fence,
    ) -> Result<u32, (u32, vk::Result)> {

I think this would be more consistent with the rest of the api.

let index = match acquire_next_image_khr(..) {
    Ok(idx) => idx,
    Err((idx, err)) => {
        // Handle the warnings, recreate the swapchain
        panic!(err == vk::Result::SUBOPTIMAL_KHR);
        idx
    }
};

[Ralith]

There is only one nonzero vk::Result for which vkAcquireNextImage returns an index; it's confusing for our API to invent an invalid index in all other cases.