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 new cl_khr_semaphore tests to verify clCreateSemaphoreWithPropertiesKHR negative results #1962

Conversation

shajder
Copy link
Contributor

@shajder shajder commented May 7, 2024

According to work plan from #1691, new clSemaphoreWrapper introduced to avoid duplication of code

Copy link
Contributor

@joshqti joshqti left a comment

Choose a reason for hiding this comment

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

CL_INVALID_SEMAPHORE_KHR is an error code. Define an invalid property local to the test, this is not correct usage of the macro

@shajder
Copy link
Contributor Author

shajder commented Jun 19, 2024

CL_INVALID_SEMAPHORE_KHR is an error code. Define an invalid property local to the test, this is not correct usage of the macro

@joshqti thanks, do you have other values on your mind ? Please keep in mind it supposed to be incorrect usage because this is negative test! CL_INVALID_SEMAPHORE_KHR is neither valid name nor property in this case.

Comment on lines 97 to 99
test_failure_error(
err, CL_INVALID_PROPERTY,
"Unexpected clCreateSemaphoreWithPropertiesKHR return");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there are two possible errors here:

  1. There is an invalid property, which would return CL_INVALID_PROPERTY.
  2. The properties do not contain the required properties to create a semaphore (specifically, the semaphore type), which would return CL_INVALID_VALUE.

We should either allow for both error conditions, or we should disambiguate the errors so only CL_INVALID_PROPERTY is returned.

Copy link
Contributor Author

@shajder shajder Jul 4, 2024

Choose a reason for hiding this comment

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

Indeed, after a second look the description of CL_INVALID_PROPERTY and CL_INVALID_VALUE slightly overlaps. I will allow both errors at the moment since correction of spec could be more time consuming. Should I create doc issue ?

paulfradgley
paulfradgley previously approved these changes Jun 25, 2024
Copy link
Contributor

@paulfradgley paulfradgley left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@kpet
Copy link
Contributor

kpet commented Jul 2, 2024

2024/07/02 teleconference: @shajder Could you rebase/resolve conflicts please?

@nikhiljnv
Copy link
Contributor

Summarizing discussion from memory subgroup call of July 9th 2024.

Per Joshua's comment, using CL_INVALID_SEMAPHORE error code as a property value is confusing.
Ben suggested two options

  • Clarify in a comment that CL_INVALID_SEMAPHORE which is an error code is actually an invalid value that can be passed to test error      condition.
    
  • Alternatively, pass a different value instead of CL_INVALID_SEMAPHORE to avoid the confusion.
    

in order to avoid using CL_INVALID_SEMAPHORE_KHR as property name or
value
@shajder
Copy link
Contributor Author

shajder commented Aug 5, 2024

Per Joshua's comment, using CL_INVALID_SEMAPHORE error code as a property value is confusing. Ben suggested two options

corrected according to mentioned suggestion

Copy link
Contributor

@nikhiljnv nikhiljnv left a comment

Choose a reason for hiding this comment

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

LGTM.

@nikhiljnv
Copy link
Contributor

Merging as discussed in memory subgroup call on August 6th 2024.

@nikhiljnv nikhiljnv merged commit d1434ae into KhronosGroup:main Aug 6, 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.

8 participants