Error module and new error handling

[DejvBayer]

Hi,

I suggest creating a vkFFT_Error module, where VkFFTResult type definition and getVkFFTErrorString(...) function would be moved. Plus the module would contain new macros VKFFT_CHECK(...) and VKFFT_CHECK_RESULT(...).

The idea is that the library could use a error handling machanism base on goto. Here is the original code:

VkFFTResult initializeVkFFT(VkFFTApplication* app, VkFFTConfiguration inputLaunchConfiguration)
{
  VkFFTResult resFFT = VKFFT_SUCCESS;

  if (app == 0)
  {
    return VKFFT_ERROR_EMPTY_app;
  }

  // ...

  resFFT = setConfigurationVkFFT(app, inputLaunchConfiguration);
  if (resFFT != VKFFT_SUCCESS)
  {
    deleteVkFFT(app);
    return resFFT;
  }
  
  // ...

  return resFFT;  
}

And here is the proposed error handling mechanism. It creates broken and brokenNoDelete labels that are used when an error occures. The benefit is that the code is more readible and you needn't to write deleteVkFFT each time.

VkFFTResult initializeVkFFT(VkFFTApplication* app, VkFFTConfiguration inputLaunchConfiguration)
{
  VkFFTResult resFFT = VKFFT_SUCCESS;

  if (app == 0)
  {
    resFFT = VKFFT_ERROR_EMPTY_app;
    goto brokenNoDelete;
  }

  // ...

  resFFT = setConfigurationVkFFT(app, inputLaunchConfiguration);
  if (resFFT != VKFFT_SUCCESS)
  {
    goto broken;
  }
  
  // ...

  return VKFFT_SUCCESS;

broken:
  deleteVkFFT(app);
brokenNoDelete:
  return resFFT;
}

However I think that we can take this idea further and define those 2 macros. In each function a resFFT variable shall be defined as it will be used to pass the error code.

1. The first one can be used to assert a boolean expression. In case it evaluates to false the resFFT variable is set to vkfftResult error and it jumps to the specified lable where the resources should be released.
2. The second checks an expression that returns a VkFFTResult. It is first evaluated into a temporary variable and if the expression was unsuccessful, it sets up the resFFT variable and jumps to the label.

#define VKFFT_CHECK(boolExpr, label, vkfftResult) \
  do { \
    if (!(boolExpr)) { \
      resFFT = (vkfftResult); \
      goto label; \
    } \
  } while (0)

#define VKFFT_CHECK_RESULT(vkfftExpr, label) \
  do { \
    VkFFTResult _resFFT = (vkfftExpr); \
    if (_resFFT != VKFFT_SUCCESS) { \
      resFFT = _resFFT; \
      goto label; \
    } \
  } while (0)

The example rewritten using those macros would be like:

VkFFTResult initializeVkFFT(VkFFTApplication* app, VkFFTConfiguration inputLaunchConfiguration)
{
  VkFFTResult resFFT = VKFFT_SUCCESS;

  VKFFT_CHECK(app != 0, brokenNoDelete, VKFFT_ERROR_EMPTY_app);

  // ...

  VKFFT_CHECK_RESULT(setConfigurationVkFFT(app, inputLaunchConfiguration), broken);
  
  // ...

  return VKFFT_SUCCESS;

broken:
  deleteVkFFT(app);
brokenNoDelete:
  return resFFT;
}

I think that this change would shorten the code very much and will also improve the overall code quality. Plus we can define macros for backend's error values like VKFFT_CHECK_RESULT_CUDA or VKFFT_CHECK_RESULT_HIP.

[DejvBayer]

I created a preview of this feature for vkFFT_InitializeApp module in:
https://github.com/DejvBayer/VkFFT/tree/goto-error-handling

I think it shortens the code quite a bit and makes it more focused on what is being done.

[DTolm]

Hello,

Sorry, I don't see how this shortens code significantly. It adds many jumps and often the VKFFT_CHECK and VKFFT_CHECK_RESULT behavior is different from the proposed base function - sometimes they have to also clear local resources. I also feel like using goto will raise quite some red flags in people's eyes. I personally like it when everything is located nearby so having an additional line but avoiding full file jumps is fine by me. If more people decide that your approach is better we can come back to discussing it in the future.

Best regards,
Dmitrii