KhronosGroup / Vulkan-Hpp

Open-Source Vulkan C++ API
Apache License 2.0
3.12k stars 305 forks source link

Inconsistent enum naming convension #1802

Closed YishiMichael closed 8 months ago

YishiMichael commented 8 months ago

When looking into some enumerant interfaces provided by Vulkan-Hpp I noticed some naming inconsistency:

enum class VendorId
{
  eVIV      = VK_VENDOR_ID_VIV,  /* Capitalized VIV: should be Viv */
  eVSI      = VK_VENDOR_ID_VSI,  /* Capitalized VSI: should be Vsi */
  eKazan    = VK_VENDOR_ID_KAZAN,
  eCodeplay = VK_VENDOR_ID_CODEPLAY,
  eMESA     = VK_VENDOR_ID_MESA,  /* Capitalized MESA: should be Mesa */
  ePocl     = VK_VENDOR_ID_POCL,
  eMobileye = VK_VENDOR_ID_MOBILEYE
};

enum class DriverId
{
  eAmdProprietary            = VK_DRIVER_ID_AMD_PROPRIETARY,
  eAmdOpenSource             = VK_DRIVER_ID_AMD_OPEN_SOURCE,
  eMesaRadv                  = VK_DRIVER_ID_MESA_RADV,
  eNvidiaProprietary         = VK_DRIVER_ID_NVIDIA_PROPRIETARY,
  eIntelProprietaryWindows   = VK_DRIVER_ID_INTEL_PROPRIETARY_WINDOWS,
  eIntelOpenSourceMESA       = VK_DRIVER_ID_INTEL_OPEN_SOURCE_MESA,  /* Capitalized MESA: should be Mesa */
  eImaginationProprietary    = VK_DRIVER_ID_IMAGINATION_PROPRIETARY,
  eQualcommProprietary       = VK_DRIVER_ID_QUALCOMM_PROPRIETARY,
  eArmProprietary            = VK_DRIVER_ID_ARM_PROPRIETARY,
  eGoogleSwiftshader         = VK_DRIVER_ID_GOOGLE_SWIFTSHADER,
  eGgpProprietary            = VK_DRIVER_ID_GGP_PROPRIETARY,
  eBroadcomProprietary       = VK_DRIVER_ID_BROADCOM_PROPRIETARY,
  eMesaLlvmpipe              = VK_DRIVER_ID_MESA_LLVMPIPE,
  eMoltenvk                  = VK_DRIVER_ID_MOLTENVK,
  eCoreaviProprietary        = VK_DRIVER_ID_COREAVI_PROPRIETARY,
  eJuiceProprietary          = VK_DRIVER_ID_JUICE_PROPRIETARY,
  eVerisiliconProprietary    = VK_DRIVER_ID_VERISILICON_PROPRIETARY,
  eMesaTurnip                = VK_DRIVER_ID_MESA_TURNIP,
  eMesaV3Dv                  = VK_DRIVER_ID_MESA_V3DV,
  eMesaPanvk                 = VK_DRIVER_ID_MESA_PANVK,
  eSamsungProprietary        = VK_DRIVER_ID_SAMSUNG_PROPRIETARY,
  eMesaVenus                 = VK_DRIVER_ID_MESA_VENUS,
  eMesaDozen                 = VK_DRIVER_ID_MESA_DOZEN,
  eMesaNvk                   = VK_DRIVER_ID_MESA_NVK,
  eImaginationOpenSourceMESA = VK_DRIVER_ID_IMAGINATION_OPEN_SOURCE_MESA,  /* Capitalized MESA: should be Mesa */
  eMesaAgxv                  = VK_DRIVER_ID_MESA_AGXV
};

I know that the enumerant names are automatically generated by first detecting possible tag names at behind. But sometimes the last segment of a name happens to collide with one of possible tags when that segment does not really represent a tag, resulting in the inconsistency above.

One approach I have observed to correctly detect tags in extension names is to parse the name attribute in the extension xml node, try to fetch the second upper-case segment (representing the tag), and parse also the depends attribute in the require xml node and fetch the second upper-case segment (the latter supersedes the former).

asuessenbach commented 8 months ago

Thanks a lot for spotting this issue! I will adjust that asap.

YishiMichael commented 8 months ago

One approach I have observed to correctly detect tags in extension names is to parse the name attribute in the extension xml node, try to fetch the second upper-case segment (representing the tag), and parse also the depends attribute in the require xml node and fetch the second upper-case segment (the latter supersedes the former).

As I dive deeper, I find such convension does not generally apply to all extension names - there are too many cases that do not follow the pattern. As a quick-and-easy fix, I now would suggest to take enum members of VendorId and DriverId as special cases that cannot have any trailing tag.

asuessenbach commented 8 months ago

You're right, there are too many exceptions that don't follow the general pattern. Another option here would be to just leave it as it is. After all, it's in there for a while, and changing that might require changes in some users code. And I don't know, if some day there might materialize some enum value that are supposed to have a trailing tag, so special case handling for those two enums does not sound like a good approach.

YishiMichael commented 8 months ago

Agree. So I'll close this issue for now.