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

Flag enum types and flags typedefs #159

Closed
kainino0x opened this issue Jun 3, 2022 · 6 comments
Closed

Flag enum types and flags typedefs #159

kainino0x opened this issue Jun 3, 2022 · 6 comments
Labels
has resolution Issue is resolved, just needs to be done

Comments

@kainino0x
Copy link
Collaborator

kainino0x commented Jun 3, 2022

Originally posted by @mrshannon in #158 (comment):

Any idea on how to improve WGPUBufferUsage vs. WGPUBufferUsageFlags?

You technically don't need WGPUBufferUsageFlags as WGPUBufferUsage will > always be wide enough for the bitwise combination because we have WGPUBufferUsage_Force32 = 0x7FFFFFFF which is effectively doing what is done for the C++ headers with:

enum class BufferUsage : uint32_t {

This is strictly true, but in Dawn's C++ bindings we also have operator overloads, so when you do wgpu::BufferUsage::CopySrc | wgpu::BufferUsage::CopyDst you get a wgpu::BufferUsage value. In C WGPUBufferUsage_CopySrc | WGPUBufferUsage_CopyDst will give you an int of some kind. So this would force callers of the API to cast back to the appropriate type.

desc.usage = static_cast<WGPUBufferUsage>(WGPUBufferUsage_CopySrc | WGPUBufferUsage_CopyDst);

Apparently, the cast is only needed in C++. C seems happy without a cast. So I guess we could define operator overloads for these types behind #ifdef __cplusplus...?

@kainino0x
Copy link
Collaborator Author

i.e.

#ifdef __cplusplus
WGPUBufferUsage operator|(WGPUBufferUsage a, WGPUBufferUsage b) {
    return static_cast<WGPUBufferUsage>(a | b);
}

and so on

@kainino0x
Copy link
Collaborator Author

FWIW, we copied this pattern from Vulkan:
#1 (comment)

in any case, whatever we do, we need to make sure it works fully in both C and C++.

@Kangz
Copy link
Collaborator

Kangz commented Jun 7, 2022

Adding operators only in C++ if that's where it causes problems would make sense. We could have something similar to Dawn's EnumClassBitmask.h.

in any case, whatever we do, we need to make sure it works fully in both C and C++.

Yeah right now webgpu.h doesn't work in C because of #84 even.

@kainino0x
Copy link
Collaborator Author

A Dawn contributor just so happened to find a pattern just like this in mingw's winnt.h: https://github.com/mirror/mingw-w64/blob/1de9cc347da5215889f6e8ff3bd4bcc820aa50bf/mingw-w64-headers/include/winnt.h#L682

#ifdef __cplusplus
#define DEFINE_ENUM_FLAG_OPERATORS(ENUMTYPE) \
extern "C++" { \
inline ENUMTYPE operator | (ENUMTYPE a, ENUMTYPE b) { return ENUMTYPE(((int)a) | ((int)b)); } \
...

@kainino0x
Copy link
Collaborator Author

meeting: agreed to do that, to patch over the difference between C and C++.

However I forgot that the original point of this issue was whether we wanted to get rid of the *Flags types, as we don't need this change otherwise (in C++, enum | enum -> int). I think we should get rid of the Flags types.

@kainino0x kainino0x added the has resolution Issue is resolved, just needs to be done label Sep 8, 2023
@kainino0x
Copy link
Collaborator Author

Obsoleted by #273 due to replacing all bitflag enums with uint64_t.

@kainino0x kainino0x closed this as not planned Won't fix, can't repro, duplicate, stale Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has resolution Issue is resolved, just needs to be done
Projects
None yet
Development

No branches or pull requests

2 participants