KhronosGroup / OpenGL-Registry

OpenGL, OpenGL ES, and OpenGL ES-SC API and Extension Registry
690 stars 276 forks source link

Distribute group attribute from <enums> tags to individual <enum> tags. #477

Closed NogginBops closed 3 years ago

NogginBops commented 3 years ago

The idea with this PR is to fix an inconsistency where most <enum> tags within an <enums> tag with a group attribute also contain a group attribute with the same group. Eg.

<enums namespace="GL" group="AttribMask" type="bitmask">
        <enum value="0x00000001" name="GL_CURRENT_BIT" group="AttribMask"/>
        ...

The group "AttribMask" is annotated both on the enums tag as well as the individual enum tags.

But there are 5 enum groups where this isn't the case.

group="RegisterCombinerPname"
group="PathRenderingMaskNV"
group="PathRenderingTokenNV"
group="SpecialNumbers"
group="ContainerType"

What this PR does currently:

There are two interesting questions relating to the remaining groups:

First, both "PathRenderingMaskNV" and "PathRenderingTokenNV" are defined on enums tags while all of the enum tags inside have their own appropriate groups, so it wouldn't make sense to add these enum groups as they don't make sense (they are only referenced in some <unused> tags for "PathRenderingTokenNV").

Second, the "SpecialNumbers" group doesn't fit the same mold as all of the other groups (unusual mix of types), so I also think that it wouldn't really make sense to distribute this into the <enum> tags. we could easily distribute it to the <enum> tags, and I think that is the solution, for some reason it initially felt weird to me.

As the <enums group=""> attributes basically become useless with this PR I wonder if it wouldn't be worth it to actually remove these attributes. The only concern I can really see is that it's likely people have special logic for the "SpecialNumbers" group that would break if it where removed, but it would be good if people could weigh in on this. Removing the group attribute from <enums> tags isn't that important and I would be fine with just leaving them, but any sort of tidiness is a win with gl.xml I feel.

Pinging some people how might have input on this (taken from #335 as there is not a ping-list for people that are affected by these types of changes): @frederikja163 @Perksey @oddhack @3b @null77 Sorry if you didn't want to get pinged 🙂

Perksey commented 3 years ago

As the attributes basically become useless with this PR I wonder if it wouldn't be worth it to actually remove these attributes.

Oh yeah this would be breaking. I think we should keep it in for historical reasons. I doubt there'll ever be any more enums with a group attribute going forward, but as you said people will likely be using this attribute meaningfully as with other attributes in the spec.

The groups blocks were removed because they were significantly outdated and to encourage extension authors to declare their groups as they make extensions (because they're not as out-of-the-way as they used to be, there's little reason not to define a group anymore) whereas I don't think the enums-level group has such issue nor is it as meaningful as groups (and the new enum-level group)

TL;DR yes we could remove it, but I don't think we should as it's not hurting anybody.

Perksey commented 3 years ago

Also you do make a good point about SpecialNumbers - I don't see any other way to define this without the enums-level group as the XML spec has no concept of const (and if we gave it one it'd be even more breaking) because all enums are added to the header as macros, so they'd be functionally equivalent anyway.

Too breaking for too little gain I think!

NogginBops commented 3 years ago

Regarding SpecialNumbers, I've changed my opinion in favor of actually distributing it into the <enum> tags. This makes it possible to ignore the <enums group=""> attribute entirely without any loss in functionality (the two remaining groups "PathRenderingMaskNV" and "PathRenderingTokenNV" don't provide any functionality and should probably be removed).

pdaniell-nv commented 3 years ago

@NogginBops is this still a work in progress?

NogginBops commented 3 years ago

All of the changes I want to make are done. The groups "PathRenderingMaskNV" and "PathRenderingTokenNV" shouldn't matter here as they are useless in practice. The question about what to about the <enums groups=""> attributes is still an open question.

I wouldn't have any problems merging this as it is. By merging this PR consumers can effectively ignore the <enums groups=""> attributes without any loss in functionality, which is nice.

Perksey commented 3 years ago

I think leaving <enums group="" as is is the best thing to do here

oddhack commented 3 years ago

@pdaniell-nv this looks fine to me.

pdaniell-nv commented 3 years ago

@oddhack this is approved to merge.