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

Fix invalid generation of the "Parameters" section #2304

Merged
merged 1 commit into from
Feb 8, 2024

Conversation

MrMetric
Copy link
Contributor

@MrMetric MrMetric commented Feb 4, 2024

In the page for PFN_vkVoidFunction, the single description sentence is split over the Parameters and Description sections, like this:

Parameters

This type is returned from command function pointer queries, and must be

Description

cast to an actual command function pointer before use.

I found that scripts/reflib.py assumes every function pointer and function prototype page has parameter documentation, so I made it check that. I tested my changes by diffing the output of ./makeSpec -spec all manhtmlpages, and found 13 affected files. First, these 3 changes seem correct to me:

  • PFN_vkVoidFunction
    • The invalid Parameters section, including its heading, is gone. The whole sentence is in the Description section.
  • vkCmdSetPerformanceMarkerINTEL
    • The Parameters section had a single sentence, with the Description section having only "Valid Usage (Implicit)" and such. The sentence has been moved to the Description section, and the Parameters heading is gone.
  • vkGetDeviceProcAddr
    • The Parameters section had two sentences that look like they're meant to be part of the description. Those sentences have been moved into the Description section, and the Parameters heading is gone.

For 8 pages, the Parameters section was empty, and the heading is now gone. This looks fine to me, but I don't know if the Parameters section is expected to be present even when it's empty. Perhaps these pages should have some information added on each function's parameters?

For 2 pages, the Parameters section had a deprecation note, with the Description section having the parameters. The note has been moved to the Description section, and the Parameters heading is gone.

These MVK pages were already wrong, and are now slightly differently wrong. They need further attention from someone more familiar with this documentation repository and its conventions.

@CLAassistant
Copy link

CLAassistant commented Feb 4, 2024

CLA assistant check
All committers have signed the CLA.

@oddhack
Copy link
Contributor

oddhack commented Feb 7, 2024

Thanks! This is great. The scripts try to infer structural information that's not explicit in the markup by assuming the style guide conventions are strictly followed, and this does a better job of inferring in these cases. I agree some of the descriptions are missing content they should have, and will take a look at the suspect pages.

BTW if you are motivated to do something like this again, you might find it useful to refer to output of the 'refpages' target, which just generates asciidoc markup under gen/refpage, rather than actually building all the refpage HTML outputs. That takes a while even on my 20-core CPU.

@oddhack
Copy link
Contributor

oddhack commented Feb 7, 2024

The Cu*NV pages are for an extension which NVIDIA has chosen not to document in the Vulkan spec. We considered it better to have at least a reference to the APIs since the XML is provided, and since it cuts down on warnings from some of our validation scripts - but can't provide spec language ourselves since it's a vendor extension. So what you've done is probably all that can be done, for now.

@oddhack oddhack added this to the Signed-off to Merge milestone Feb 8, 2024
@oddhack oddhack merged commit 0d65197 into KhronosGroup:main Feb 8, 2024
11 checks passed
@MrMetric MrMetric deleted the fix_invalid_parameters_section branch February 8, 2024 03:44
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.

3 participants