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

Add overridable attributes to all types (defaulting to empty) #182

Merged
merged 1 commit into from
May 24, 2023

Conversation

Kangz
Copy link
Collaborator

@Kangz Kangz commented May 16, 2023

@litherum PTAL at the second commit, would this work for you?

@Kangz Kangz requested a review from austinEng May 16, 2023 14:19
@austinEng
Copy link
Collaborator

while you're at it; you may consider adding a WGPU_NULLABLE attribute too?

@Kangz
Copy link
Collaborator Author

Kangz commented May 17, 2023

I'll add WGPU_NULLABLE if we decide to move forward after the discussions in #179

@litherum
Copy link

+1 to nullable <3

Yes, this patch looks perfect.

@Kangz Kangz force-pushed the atttributes branch 2 times, most recently from 97ec8e0 to 785b47d Compare May 22, 2023 10:12
@Kangz
Copy link
Collaborator Author

Kangz commented May 22, 2023

PTAL again, I had to bundle a couple changes that would have been annoying to split up.

Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM w/ non-blocking comments


typedef struct WGPUComputePassTimestampWrite {
WGPUQuerySet querySet;
uint32_t queryIndex;
WGPUComputePassTimestampLocation location;
} WGPUComputePassTimestampWrite;
} WGPUComputePassTimestampWrite WGPU_STRUCTURE_ATTRIBUTE;

typedef struct WGPUConstantEntry {
WGPUChainedStruct const * nextInChain;
char const * key;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might also want a NONNULL for all of the pointers that aren't nullable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's would be a lot of noise, but maybe? What do others think of adding a WGPU_NONNULL

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#define WGPU_FUNCTION_ATTRIBUTE
#endif
#if !defined(WGPU_NULLABLE)
#define WGPU_NULLABLE
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want the header to use the clang extension (and maybe other extensions) by default if available, or just leave this for people who want to take advantage of it?

typedef void (*WGPUProc)(void) WGPU_FUNCTION_ATTRIBUTE;
typedef void (*WGPUQueueWorkDoneCallback)(WGPUQueueWorkDoneStatus status, void * userdata) WGPU_FUNCTION_ATTRIBUTE;
typedef void (*WGPURequestAdapterCallback)(WGPURequestAdapterStatus status, WGPUAdapter adapter, char const * message, void * userdata) WGPU_FUNCTION_ATTRIBUTE;
typedef void (*WGPURequestDeviceCallback)(WGPURequestDeviceStatus status, WGPUDevice device, char const * message, void * userdata) WGPU_FUNCTION_ATTRIBUTE;

typedef struct WGPUChainedStruct {
struct WGPUChainedStruct const * next;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be nullable

webgpu.h Show resolved Hide resolved
 - Also adds WGPU_NULLABLE instead of the /* nullable */ comments
 - Also adds WGPUFeatureName_Float32Filterable
 - Also adds struct forward declarations and move funtion pointers
   typedefs before the struct definitions, so that struct can contain
   function pointers.

Fixes webgpu-native#179
Fixes webgpu-native#119
@kainino0x
Copy link
Collaborator

Rebased past #181, let's do the rest as followup

@kainino0x kainino0x merged commit 8f819a4 into webgpu-native:main May 24, 2023
@Kangz Kangz deleted the atttributes branch May 24, 2023 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants