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 a warning comment to the <group> tags explaining that they may be out of date #291

Merged
merged 5 commits into from
Sep 12, 2019

Conversation

oddhack
Copy link
Contributor

@oddhack oddhack commented Aug 9, 2019

Fixes #280
Not in the sense of "making the group tags complete", but in the sense of "explaining that they aren't complete and we would need help in changing that".

xml/gl.xml Show resolved Hide resolved
xml/gl.xml Show resolved Hide resolved
@frederikja163
Copy link
Contributor

You might wanna take a look at #292 before you merge this PR

@Perksey
Copy link
Contributor

Perksey commented Aug 10, 2019

#292 is a great maintenance PR, but I still think we should keep this warning in the spec as without Khronos' backing of the groupings, it won't be hard for PRs to slip through which degrade the validity of the groupings.

xml/gl.xml Show resolved Hide resolved
@Perksey
Copy link
Contributor

Perksey commented Aug 21, 2019

Side note: Has the Khronos group investigated moving to the Vulkan specification format? (i.e. merging all enum groupings into enums with namespaces)

@oddhack
Copy link
Contributor Author

oddhack commented Aug 22, 2019

@Perksey the Vulkan/OpenXR schema has some significant differences although it started off with the GL stuff, so it would be a lot of work to switch over. Not currently planned.

@Perksey
Copy link
Contributor

Perksey commented Aug 22, 2019 via email

@oddhack
Copy link
Contributor Author

oddhack commented Aug 22, 2019

There are RNC schemas for both. Converting the XML file would be a small part of the problem, though.

@pdaniell-nv
Copy link
Contributor

Thanks for the input on this PR so far. Any other comments before we accept this change? Thanks.

xml/gl.xml Show resolved Hide resolved
@Perksey
Copy link
Contributor

Perksey commented Sep 3, 2019

After that change has been applied, this PR has my green light.

@oddhack
Copy link
Contributor Author

oddhack commented Sep 10, 2019

@pdaniell-nv I think this should be OK now? AFAICT everyone's comments / requests have been incorporated.

@pdaniell-nv
Copy link
Contributor

@oddhack yep looks good. Approved to merge.

Copy link
Contributor

@Perksey Perksey left a comment

Choose a reason for hiding this comment

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

LGTM

@oddhack oddhack merged commit 3e75f41 into master Sep 12, 2019
@oddhack oddhack deleted the 280-group-warning branch December 12, 2019 10:25
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.

State Khronos' official stance on enum groupings
5 participants