Skip to content

Add properties parameter to all command-buffer commands#305

Merged
bashbaug merged 2 commits intoKhronosGroup:mainfrom
EwanC:ewan/command_properties
Sep 6, 2024
Merged

Add properties parameter to all command-buffer commands#305
bashbaug merged 2 commits intoKhronosGroup:mainfrom
EwanC:ewan/command_properties

Conversation

@EwanC
Copy link
Copy Markdown
Contributor

@EwanC EwanC commented Aug 15, 2024

Updates to compile with header change KhronosGroup/OpenCL-Headers#260

@bashbaug
Copy link
Copy Markdown
Contributor

I think it would be better to keep the current functions as they currently are, with no "properties" argument, and then add overloads with a "properties" argument. This has two main advantages:

  1. Any code previously written for the older functions continues to work without any changes.
  2. When the properties argument is not required - which will be the common case initially, at least - we won't need to pass as many arguments.

We could even skip adding the overloads with the "properties" argument for now, at least for the command buffer APIs where no properties are currently defined.

@EwanC EwanC force-pushed the ewan/command_properties branch from f5d6b98 to 5a6632c Compare August 16, 2024 07:37
@EwanC
Copy link
Copy Markdown
Contributor Author

EwanC commented Aug 16, 2024

I think it would be better to keep the current functions as they currently are, with no "properties" argument, and then add overloads with a "properties" argument. This has two main advantages:

1. Any code previously written for the older functions continues to work without any changes.

2. When the properties argument is not required - which will be the common case initially, at least - we won't need to pass as many arguments.

We could even skip adding the overloads with the "properties" argument for now, at least for the command buffer APIs where no properties are currently defined.

Avoiding breaking user code if we can avoid it makes sense to me. Updated and left out the overloaded properties variant.

Copy link
Copy Markdown
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM.

@EwanC EwanC marked this pull request as ready for review September 5, 2024 16:36
@bashbaug
Copy link
Copy Markdown
Contributor

bashbaug commented Sep 6, 2024

Merging as discussed in the September 3rd teleconference + email.

The CI failures appear to be real, but are unrelated to these code changes.

@bashbaug bashbaug merged commit 6db44b8 into KhronosGroup:main Sep 6, 2024
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.

2 participants