Defining enums and bitmasks using enum vs. as constants

[kainino0x]

Originally posted by @kainino0x in #270 (comment)

Currently extended bitflag entries are similar to enums, but I think we should change generation of all bitflag entries to be static const T N = V instead of enum, because we need to make them 64bit anyway. Vulkan does the same for 64bit bitflags

Ah... unfortunate. I dug a little to figure out why this was the case in Vulkan. Clang makes it clear - and this is also the reason Force32 is 0x7fffffff instead of 0xffffffff.

warning: ISO C restricts enumerator values to range of 'int' (4294967295 is too large) [-Wpedantic]

C++ seems to have no such complaints for enum class, so Dawn should still be able to do the thing we do in C++ to make strongly-typed bitflags from enum class.
C++ also seems to have no complains for enum, which means we could use enum behind #ifdef __cplusplus, for added type safety. Doing so would be consistent with #159, where we were going to do the operator| thing to make C++ happy with our usage of enums as bitflags. But having separate C and C++ definitions of entire enums is annoying, and also sketchy - it almost certainly breaks ABI compatibility between the C and C++ versions.
So I think we are going to have to scrap the operator| thing and just use 64-bit constants. Too bad.

the downside being we loose exhaustive switches in C++ over new entries

Actually this raises another interesting point - if we extend enums outside the enum definition, then it becomes easy to write a switch that passes the exhaustiveness check, but is not actually exhaustive. Of course, this was always true of bitflags, and generally true if you cast numbers to enums, but those are corner cases.

This is probably fine, it just adds a little bit of a false sense of security. Almost certainly better to get some warnings than no warnings.
Most enums won't be commonly in application code anyway. The enums we do pass out to the application may get extended, but for most enums, we should never start returning new values to existing applications unless we enable some feature, so exhaustivity over the core enum is all that really matters.
For reference, I think the only enums where we may suddenly start passing out new values to the application are FeatureName (in AdapterEnumerateFeatures, but not DeviceEnumerateFeatures) and WGSLFeatureName (in GetInstanceFeatures or whatever we end up calling it). Not SType, I think, if we solve #272.

Maybe... after we stabilize, we should just never change the enum definitions and only extend them using static const? (maybe even rename Force32 to indicate it's there to make the enum extensible/non-exhaustive, as anyone writing exhaustive switches will end up having to type Force32)

(Aside, apparently there is a gcc/clang flag that checks for exhaustive switches even with default. I am now tempted to try this in Dawn. https://brevzin.github.io/c++/2019/08/01/enums-default/#the-warning-that-exists)

[kainino0x]

Mar 14 meeting:

• Bitmasks
  • RM: What does Vulkan-Hpp do?
    • https://github.com/KhronosGroup/Vulkan-Hpp/blob/695de1bc3a73cd5f5305feea49eee82e2ecbd21b/vulkan/vulkansc\_enums.hpp#L2824C39-L2824C60
  • Do the same as Vulkan, just typedef as uint64_t and define values as static const
• Other enums
  • KN: Better to have some warnings than no warnings?
  • AE: (agree)
  • RM: Document it in the header
  • Don’t switch to using uint32_t, keep using enum
  • “after we stabilize, we should just never change the enum definitions and only extend them using static const?”
    • No
  • Rename Force32 to indicate non-exhaustive?
    • KN: “Force32” plus documentation (on the Force32 value) should be enough to indicate it’s non-exhaustive?
    • Don’t rename.
    • In C++ enum class, there’s no Force32. Possible to write an exhaustive switch over just the normal values.
    • AE: Enum class would actually be exhaustive because it includes extensions?
    • KN: Not if we make C++ bindings independent of implementation, then the bindings would need to know EVERY extension from every implementation.
    • Not a webgpu.h problem
• RM: Is there a better way to extend enums (in another header file) than to define static consts?
  • KN: Don’t think so.

[kainino0x]

• Do the same as Vulkan, just typedef as uint64_t and define values as static const

I just remembered we previously chose to use #define instead of static const in #34. It seems the reason was only that it was more idiomatic, but I guess the same argument applies here, so we should think about which to use.

[kainino0x]

Actually Vulkan does use static const so that's probably enough confirmation for us to do it that way too.

[kainino0x]

Also... naming question, should we drop "Flags" from the name since there is now just one type (the uint64_t typedef) instead of two (the uint32_t typedef and the enum)?

e.g. WGPUBufferUsageFlags -> WGPUBufferUsage

[austinEng]

IMO, we should drop "Flags" to match the naming in the JS API

[kainino0x]

WebIDL has both names (though the "Flags" one is not exposed to JS):

typedef [EnforceRange] unsigned long GPUBufferUsageFlags;
[Exposed=(Window, Worker), SecureContext]
namespace GPUBufferUsage {
    const GPUFlagsConstant MAP_READ      = 0x0001;

I think the C equivalent would arguably be:

typedef uint64_t GPUBufferUsageFlags;
static const GPUBufferUsageFlags GPUBufferUsage_MapRead = 0x0001;

but of course the "Flags" name becomes exposed to the application unlike in JS.

[kainino0x]

Jul 18 meeting:

• Bikeshed the bitmask typedef name. Brainstorm:
• GPUBufferUsage_MapRead
• GPUBufferUsageFlags
• GPUBufferUsage
• GPUBufferUsages
• Use the naming conventions in bold above