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

Bad handling of type tag with name attribute, rather than name tag #1462

Closed
oddhack opened this issue Dec 6, 2022 · 4 comments · May be fixed by #1463
Closed

Bad handling of type tag with name attribute, rather than name tag #1462

oddhack opened this issue Dec 6, 2022 · 4 comments · May be fixed by #1463

Comments

@oddhack
Copy link

oddhack commented Dec 6, 2022

See internal MR 5593 for details but the gist is that we are trying to use an empty tag of form

<type category="funcpointer" name="PFN_vkGetInstanceProcAddr"></type>

to replace the explicit (re)definition introduced by the LUNARG extension in the latest spec update. Unfortunately, this causes the following error from the hpp-generate CI step:

VulkanHppGenerator: Spec warning on line 905: unknown attribute !
caught exception: VulkanHppGenerator: Spec error on line 905: missing required element

Note that this has always been legal XML syntax, although accepting it may lead to another problem with Vulkan-Hpp, since there is then no explicit definition of the PFN type available - the PFN is just emitted along with the vkGetInstanceProcAddr prototype in the C headers. This is a new use case we haven't encountered before, all the other PFN types in the XML are explicit callbacks.

@oddhack
Copy link
Author

oddhack commented Dec 7, 2022

FYI - we are in process of abandoning internal MR 5593 in preference to 5596, which introduces a new type for the PFN. So the fix in #1463, while still appropriate, isn't immediately required. Would appreciate feedback on internal MR 5596 however.

@asuessenbach
Copy link
Contributor

That is, #1463 isn't needed at all? I'll close it then. It would just introduce unneeded complexity.
Besides that, I think, using an attribute for something like "name" seems to be more appropriate to me than a tag name (or an element). After all, there's supposed to be exactly one "name" per thing, and that name should probably not contain any other information. But, of course, that should be the standard, not the exception. Just my opinion.
Just replacing <PFN_vkGetInstanceProcAddr> by <PFN_vkGetInstanceProcAddrLUNARG> would seamlessly work with the generator, but would break all code that's explicitly using <PFN_vkGetInstanceProcAddr>!

@oddhack
Copy link
Author

oddhack commented Dec 9, 2022

That is, #1463 isn't needed at all? I'll close it then. It would just introduce unneeded complexity. Besides that, I think, using an attribute for something like "name" seems to be more appropriate to me than a tag name (or an element). After all, there's supposed to be exactly one "name" per thing, and that name should probably not contain any other information. But, of course, that should be the standard, not the exception. Just my opinion.

It may be needed in the future, because this is part of the schema. In general we use <name> tags because the name has to be part of the tag content in any event, and having e.g. <type name="VkFoo">...typedef void VkFoo;</type> is redundant and error-prone - but there are occasional cases where a tag needs a name, so it can be used as a dependency, yet the name isn't needed in the tag content (usually something like external header dependencies which define types).

Just replacing <PFN_vkGetInstanceProcAddr> by <PFN_vkGetInstanceProcAddrLUNARG> would seamlessly work with the generator, but would break all code that's explicitly using <PFN_vkGetInstanceProcAddr>!

That is in fact what we did, but I don't think it's going to break anything given the very specialized use of this extension - the extension author signed off on it.

@asuessenbach
Copy link
Contributor

and having e.g. ...typedef void VkFoo; is redundant and error-prone

I assume, you're referencing some vk.xml lines like
<type category="basetype">typedef <type>uint32_t</type> <name>VkSampleMask</name>;</type>

For such a line, I would have expected a description like
<type category="basetype" type="uint32_t" name="VkSampleMask">

While the current line is kind of "annotated C code", the latter would be a bit more language independent.
But, well, it is what it is, and it is probably far too demanding to ask for such a fundamental change in vk.xml ;)

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 a pull request may close this issue.

2 participants