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

Added negative tests for clCommandCopy[Buffer, BufferRect, BufferToImage]KHR #1941

Conversation

kamil-goras-mobica
Copy link
Contributor

According to description #1668

@kamil-goras-mobica kamil-goras-mobica marked this pull request as draft April 8, 2024 12:46
@kamil-goras-mobica kamil-goras-mobica marked this pull request as ready for review April 9, 2024 05:54
{
CommandBufferCopyDifferentContexts(cl_device_id device, cl_context context,
cl_command_queue queue)
: BasicCommandBufferTest(device, context, queue), in_mem_ctx(nullptr),
Copy link
Contributor

Choose a reason for hiding this comment

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

You can put all these nullptr initialisers as designated initialisers on the declaration line rather than list them on the constructor which will be more cumbersome.

return CL_SUCCESS;
}
};
};
Copy link
Contributor

Choose a reason for hiding this comment

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

trailing semicolon ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@silverclaw Yes, it is closing anonymous namespace, I have done it the same way as other tests are done in this directory.

Copy link
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.

Mostly LGTM. I can pass these tests on some implementations with "enhanced error checking" using the command buffer emulation layer, but I think it's exposed some bugs in other implementations. 😁

@bashbaug
Copy link
Contributor

Note: there are some merge conflicts that will need to be resolved, also.

bashbaug
bashbaug previously approved these changes May 21, 2024
Copy link
Contributor

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

Seeing tests fail on an implementation without image support due to calling image APIs without querying device support first and then skipping

@kamil-goras-mobica
Copy link
Contributor Author

@EwanC added checking for IMAGE_SUPPORT, divided tests by copyBuffer* and copyImage func

Copy link
Contributor

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

LGTM

@neiltrevett neiltrevett merged commit d379b58 into KhronosGroup:main Jun 11, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants