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

generate and use an extension dictionary #1131

Merged
merged 4 commits into from
Apr 3, 2024

Conversation

bashbaug
Copy link
Contributor

@bashbaug bashbaug commented Apr 2, 2024

fixes #1093

Generates and uses a new asciidoctor dictionary for extension names. I am currently generating three different dictionaries:

  1. A dictionary with local links, for use in the API spec.
  2. A dictionary with no links, currently used for all other specs including the online reference pages.
  3. A dictionary with ful links. This is not currenlty used, but I decided to keep it around in case we want to use it in more place.

Note that we already define asciidoctor attributes with the unmodified extension names to control whether to include the extension in the spec, e.g. {cl_khr_fp64}, so I've currently suffixed the extension names in the dictionaries with _EXT. This means that the attribute for the extension name is e.g. {cl_khr_fp64_EXT}. This is similar to types, which also have a suffix, but it seems rather error prone since if you use the un-suffixed extension name there are no errors or warnings and the extension name just doesn't appera in the spec. We should think whether there is a better long-term solution.

@bashbaug
Copy link
Contributor Author

bashbaug commented Apr 2, 2024

Discussed in the April 2nd teleconference. OK to merge after conflicts are resolved.

@oddhack
Copy link
Contributor

oddhack commented Apr 2, 2024

Would cranking :attribute-missing: up to error (I believe it's set to warn right now) address the silent failure issue?

@oddhack
Copy link
Contributor

oddhack commented Apr 2, 2024

Contextually, I can think of only three ways of doing this rewriting:

  • attributes pregenerated, as CL does
  • macros interpreted at build time by reference to Ruby data structures generated from the XML, as Vulkan does
  • A treeprocessor extension which looks for words (w/o markup of any kind) and interprets them like the macros (this would be exceedingly error-prone - having the build literally rewrite your words when you didn't ask it to just seems like a Bad idea)

What I found most confusing about the attributes was the inconsistency of the naming scheme - some have _TYPE appended and some do not. Just changing the naming scheme to a consistent, perhaps prefix scheme (ANCHOR_clblah, LINK_clblah, etc.) would address that, but of course any change will be very intrusive to the spec markup.

@oddhack
Copy link
Contributor

oddhack commented Apr 2, 2024

BTW, if you did contemplate using the macro approach, be aware that what Vulkan does is more complex than need be - we have a lot of different macros for different types of APIs but these days, there's enough information at runtime that we could simplify to just 'api:any-api-or-extension-name' for a link or extension name. Haven't done that for the same reason of being a huge, intrusive markup change.

@bashbaug
Copy link
Contributor Author

bashbaug commented Apr 3, 2024

Would cranking :attribute-missing: up to error (I believe it's set to warn right now) address the silent failure issue?

I don't think it would, unfortunately, because AFAICT the extension name without any suffix is defined as an attribute by the build system, in gen/specattribs.adoc, the attribute just doesn't have any value so it doesn't generate any text in the spec. Maybe we could simply update the Makefile to generate attributes with some text instead, say "OOPS!", and that way at least the incorrect attribute usage wouldn't be silent?

The build script could also generate a prefixed or suffixed attribute instead, and we could use that in our asciidoctor ifdefs, but that's just moving the problem.

For now I've been using a Visual Studio Code regular expression to find un-suffixed attributes. This is a manual process, but at least it gives me confidence the current specs are OK: \{(cl_khr_[^{}]+(?<!_EXT))\}.

What I found most confusing about the attributes was the inconsistency of the naming scheme - some have _TYPE appended and some do not.

Yeah, I wasn't expecting to need any suffix initially, since these are all C identifiers, but unfortunately asciidoctor attributes are not case sensitive so there is no way otherwise to differentiate between the enum CL_FLOAT and the type cl_float (there were a few others, too).

@bashbaug
Copy link
Contributor Author

bashbaug commented Apr 3, 2024

CI is clean, conflicts are resolved. Merging as discussed in the April 2nd teleconference.

@bashbaug bashbaug merged commit c285aa9 into KhronosGroup:main Apr 3, 2024
2 checks passed
@bashbaug bashbaug deleted the extension-dictionaries branch April 3, 2024 00:29
@oddhack
Copy link
Contributor

oddhack commented Apr 3, 2024

I don't think it would, unfortunately, because AFAICT the extension name without any suffix is defined as an attribute by the build system, in gen/specattribs.adoc, the attribute just doesn't have any value so it doesn't generate any text in the spec. Maybe we could simply update the Makefile to generate attributes with some text instead, say "OOPS!", and that way at least the incorrect attribute usage wouldn't be silent?

Would it be helpful to just define the attribute's value as its name, or an xref to the extension anchor in the appendix? Trivial to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

bad extension link appearance in the online reference pages
2 participants