Skip to content

Conversation

@bashbaug
Copy link
Contributor

These changes are consistent with the PR for the OpenCL headers, see: KhronosGroup/OpenCL-Headers#65

The extension spec is a touch out-of-date on github, but can be found here. I will be updating it to the revision described by these changes (or newer) shortly.

I have scripts that use the API definitions so I am reasonably confident that they are correct. I'm less confident regarding the typedefs and enum changes, but I have verified that the OpenCL specs still build correctly with these changes.

@Kerilk
Copy link
Contributor

Kerilk commented Jan 23, 2020

Hello,

In this PR you wrote the void keyword as <type>void</type>. Until now, in the xml file, the type markup was not used around void.

@bashbaug
Copy link
Contributor Author

@oddhack, can you provide advice when the <type> markup should be used, and specifically whether it should be used around void? I've removed the type markup around void for my changes, since that's consistent with most usage of void in the XML file, but there are still a few other places where the type markup is still used. Thanks!

@oddhack
Copy link
Contributor

oddhack commented Jan 24, 2020

@oddhack, can you provide advice when the <type> markup should be used, and specifically whether it should be used around void?

We use it consistently in the Vulkan and XR XML now. It is not necessary for C header generation and our other internal uses, though, because there's no actual dependency to satisfy to define the type before use. That is the purpose of the <type> tag. So I'd say it's your call to use it or not, but best to be consistent so the question doesn't keep coming up.

Enum values 0x417E and 0x417F are used, so only the range
0x4170-9x417D are unused.
@bashbaug
Copy link
Contributor Author

@Kerilk thanks, I fixed the inconsistency in the 0x4170 enum range also.

I'd like to address the <type> markup inconsistency as a separate PR though. I've filed #196 to track this.

@Kerilk
Copy link
Contributor

Kerilk commented Jan 24, 2020

@bashbaug You're welcome. I deleted the comment because I have a whole list of them and it will really be out of the scope of the pull request.

Copy link
Contributor

@alycm alycm left a comment

Choose a reason for hiding this comment

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

LGTM.

@bashbaug
Copy link
Contributor Author

Just FYI, I have the type markup changes done to address #196, but they build upon this change, so I'd like to merge this first.

@alycm alycm merged commit 639ae4c into KhronosGroup:master Jan 28, 2020
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