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 missing handle comment for some VkObjectType values #1379

Closed
wants to merge 1 commit into from
Closed

Add missing handle comment for some VkObjectType values #1379

wants to merge 1 commit into from

Conversation

DadSchoorse
Copy link
Contributor

For most VkObjectType values there's a comment with the matching handle name. Since they are the most elegant way to get the object type for a handle, I plan to use these comments to generate parts of Wine's vulkan code.

This PR adds comments to VkObjectType values that previously didn't have any.

@CLAassistant
Copy link

CLAassistant commented Oct 12, 2020

CLA assistant check
All committers have signed the CLA.

@TomOlson
Copy link

@DadSchoorse, this seems very fragile to us - naming a field "comment" implies that it is ignorable (not consumed by anything but human eyes) and optional (nothing breaks if you leave it out). If code is going to depend on it we will need to add CI to make sure we don't forget it when we add new types, et cetera, and we'd rather not.

The style guide specifies a fixed mapping between object type names and enum values, and we guarantee to enforce it in new types. Probably obvious by inspection, but the rule is "delete OBJECT_TYPE_ and camel-case what's left, except capitalize the extension suffix if there is one". We do that in at least one internal script. The only slightly crufty bit is recognizing extension suffixes, but vk.xml contains a complete list. We recommend getting the type names that way.

@oddhack
Copy link
Contributor

oddhack commented Oct 15, 2020

As the XML maintainer / schema creator, agreed with @TomOlson. Comments can contain anything. While it has happened that we make an error in the naming scheme Tom describes, when we realize that we fix it by making the old, incorrect name an alias of the new, correct name.

@DadSchoorse
Copy link
Contributor Author

The style guide specifies a fixed mapping between object type names and enum values, and we guarantee to enforce it in new types. Probably obvious by inspection, but the rule is "delete OBJECT_TYPE_ and camel-case what's left, except capitalize the extension suffix if there is one". We do that in at least one internal script. The only slightly crufty bit is recognizing extension suffixes, but vk.xml contains a complete list. We recommend getting the type names that way.

I considered this option, but it also seems fragile to me:
It needs the guarantee that no handle name contains any capitalized abbreviation.
Since struct names don't strictly follow this (e.g. VkGeometryAABBNV, VkPhysicalDeviceTextureCompressionASTCHDRFeaturesEXT) I concluded that future handle names might also contain completely capitalized parts.

@TomOlson
Copy link

@DadSchoorse that's a fair point - there are structure type names that violate the style guide, and we should by our rules create aliases for them. I'll raise an internal issue.

It's still the case that aside from 'comment="Optional"' in some of the query types, there are no other places in vk.xml where there is information in comment attribute values that looks like it is meant to have machine-readable semantics, and I expect the WG will want to keep it that way. For object types, string conversion does work today. I'll ping the WG and see what they want to do.

@TomOlson TomOlson self-assigned this Oct 16, 2020
DadSchoorse added a commit to DadSchoorse/wine that referenced this pull request Oct 27, 2020
We shouldn't do that according to the Vulkan xml maintainer.
KhronosGroup/Vulkan-Docs#1379

Signed-off-by: Georg Lehmann <dadschoorse@gmail.com>
@expipiplus1
Copy link
Contributor

expipiplus1 commented Oct 28, 2020

There is a table generated in Vulkan-Docs/gen/refpage/VkObjectType.txt which contains this mapping, how is this generated (looks like it's written out by hand in chapters/debugging.txt and chapters/VK_EXT_debug_report.txt)?

(Note however that this table is incomplete, the missing rows are PrivateDataSlotEXT and DeferredOperationKHR).

DadSchoorse added a commit to DadSchoorse/wine that referenced this pull request Oct 29, 2020
We shouldn't do that according to the Vulkan xml maintainer.
KhronosGroup/Vulkan-Docs#1379

Signed-off-by: Georg Lehmann <dadschoorse@gmail.com>
Signed-off-by: Liam Middlebrook <lmiddlebrook@nvidia.com>
Signed-off-by: Alexandre Julliard <julliard@winehq.org>
SveSop pushed a commit to SveSop/wine that referenced this pull request Oct 31, 2020
We shouldn't do that according to the Vulkan xml maintainer.
KhronosGroup/Vulkan-Docs#1379

Signed-off-by: Georg Lehmann <dadschoorse@gmail.com>
Signed-off-by: Liam Middlebrook <lmiddlebrook@nvidia.com>
Signed-off-by: Alexandre Julliard <julliard@winehq.org>
@oddhack
Copy link
Contributor

oddhack commented Nov 9, 2020

After some internal discussion we're intending to add an explicit new attribute on the handle <type> tags showing the corresponding OBJECT_TYPE token, and check the spelling consistency in CI. Also the table omission @expipiplus1 noted above will be fixed. Generating that table will be possible but we'll probably defer that part from the initial fixes. So this PR can probably be closed when we publish the XML updates in a couple of weeks.

@oddhack
Copy link
Contributor

oddhack commented Nov 30, 2020

This should be fixed in the 1.2.163 spec update. There is a new 'objtypeenum' attribute on handle types specifying the corresponding OBJECT_TYPE enum.

@oddhack oddhack closed this Nov 30, 2020
oddhack added a commit that referenced this pull request Dec 3, 2020
Followup to #1379. There is now a semantically meaningful schema
attribute mapping handles to corresponding VK_OBJECT_TYPES enum names.
This removes the comments which were previously being used by some
downstream consumers of the XML to infer this information on the basis
that (a) they are not defined to be usable for this purpose (b) they are
incomplete and (c) we have no intention of maintaining them in the
future when new handle types are defined.
@oddhack
Copy link
Contributor

oddhack commented Dec 3, 2020

@DadSchoorse @SveSop @expipiplus1 please take a look at #1412. We don't want to just yank the comment= attributes out from under people's feet before they can switch over, but as there is a meaningful tagging scheme for this information now, we do want to remove them pretty soon - this is a chance for feedback.

@expipiplus1
Copy link
Contributor

Thanks, @oddhack, that's very considerate. I'm not using these comments myself so of course this is OK with me :)

@DadSchoorse
Copy link
Contributor Author

DadSchoorse commented Dec 3, 2020

@oddhack I removed the code from Wine that uses the comments.

oddhack added a commit that referenced this pull request Dec 12, 2020
…#1412)

Followup to #1379. There is now a semantically meaningful schema
attribute mapping handles to corresponding VK_OBJECT_TYPES enum names.
This removes the comments which were previously being used by some
downstream consumers of the XML to infer this information on the basis
that (a) they are not defined to be usable for this purpose (b) they are
incomplete and (c) we have no intention of maintaining them in the
future when new handle types are defined.
yuiiio pushed a commit to yuiiio/wine-lol-src that referenced this pull request Apr 1, 2021
We shouldn't do that according to the Vulkan xml maintainer.
KhronosGroup/Vulkan-Docs#1379

Signed-off-by: Georg Lehmann <dadschoorse@gmail.com>
Signed-off-by: Liam Middlebrook <lmiddlebrook@nvidia.com>
Signed-off-by: Alexandre Julliard <julliard@winehq.org>
yuiiio pushed a commit to yuiiio/wine-lol-src that referenced this pull request Apr 22, 2021
We shouldn't do that according to the Vulkan xml maintainer.
KhronosGroup/Vulkan-Docs#1379

Signed-off-by: Georg Lehmann <dadschoorse@gmail.com>
Signed-off-by: Liam Middlebrook <lmiddlebrook@nvidia.com>
Signed-off-by: Alexandre Julliard <julliard@winehq.org>
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.

None yet

5 participants