Closed NogginBops closed 1 year ago
@Perksey do you have thoughts on this? I think you're probably the main expert on all things enum group related.
Thanks for filing this, I'll have a look through the attached file tomorrow and figure out how many groups we're talking, I agree on the premise but want to make sure we can get all the information that the groups convey elsewhere in the spec!
Disagree on changes here. While yes these parameters take enums but do not have the type defined as such, changes of the types here would propagate to the header files and I don't think changing parameter types in the headers for the sake of group correctness is worth it.
GLuint texture: Texture
GLuint* fences: FenceNV
Instances of these are superseded by the kind attribute, happy for these group attributes to be removed.
GLint drawbuffer: DrawBufferName
This should be a class
attribute (class="draw buffer"
) to be honest if we wanted to keep this information, but in any case happy for these group attributes to be removed.
GLubyte* return value: String
Support removal. Technically there's no way for a downstream consumer to know this is a string (see #363) but in practice I think XML users have all the info they need.
For everything else, there's nothing here that jumps out at me an inherently dangerous/wrong to remove so I'm fine with this. It's a big change, but I think where we've landed in #481 is that big changes are fine considering we don't have a stable baseline today.
TL;DR: happy with everything in the "Invalid groups", but would rather no changes to "Valid enums but invalid types".
Valid enums but invalid type
Currently, there is no good way to automatically detect them when parsing XML, because of other uses of the group
attribute, like group=Texture
.
If the resolution to this issue is to deprecate and then remove all uses of the group
attribute, other than enum groupings, that's also fine.
But at the same time, a fix for lack of information is more robust if done in both directions, as it can then be tested by each binding in their own way when re-parsing XML (for instance I output this kind of info to the .log file, so I see it in git changes after pulling this repo).
And while I understand how everyone values compatibility here, it's not right how old typo's aren't just unfixable in old bindings, but also propagate to new ones unless fixed in each binding manually.
I've thought of this a few times since #363, and, if official bindings cannot be touched, then maybe these typos should be fixed in an XML-only way? (note replace=
)
<command>
<proto group="String">const <ptype replace="GLchar">GLubyte</ptype> *<name>glGetString</name></proto>
<param group="StringName"><ptype>GLenum</ptype> <name>name</name></param>
<glx type="single" opcode="129"/>
</command>
Instances of these are superseded by the kind attribute
The 2 examples you've shown should only have the class
attribute instead.
All group=Texture
already have it - since #428. All that is left is to remove redundant (and probably unusable since enum groupings) group=Texture
.
Things like group=FenceNV
were described in the header of #428, which everyone (including me) forgot about.
Changing argument types from GLint
to GLenum
should not be binary or source breaking as they resolve to the same type. Though it seems like there are some places where GLint
unfortunately is the "correct" type, see https://github.com/KhronosGroup/OpenGL-Registry/pull/283#issuecomment-504446128.
This is unfortunate as that means we can't change signature from GLint
to GLenum
.
Unless you think it's ok to change the type to GLenum
as it still allows users to pass in numbers as the arguments. @oddhack
Guessing there is some type of verification thing that would be affected by the change? as it wouldn't be a binary nor source breaking change.
But as @SunSerega said, we could add some information to the xml which allows users to get this information. Not entirely convinced that a replace="GLchar"
attribute is the solution to go for. For the case of glTexImage2D
we would potentially need to have it conditioned on the GL version.
If the resolution to this issue is to deprecate and then remove all uses of the group attribute, other than enum groupings, that's also fine.
I believe this is the way forward yes.
an XML-only way? (note replace=)
I don't think this adds value and instead just clutters the specification. Don't get me wrong, it'd be useful, but for #363 specifically that function is the exception not the rule. Likewise, modifying the function signature after it has been specified, approved, and shipped should once again be the exception not the rule.
Flipping the question a bit here, why would we make it illegal for enum values to be passed to non-enum parameters? At the end of the day, all of these enums are just macros for specific numbers and all of these parameters accept numbers. There are also some cases where some parameters accept both enumerated values and non-enumerated values, or cases where the enumerated value is actually just a well-known constant. I don't think imposing restrictions makes sense.
And in any case, a header change like that will likely have to go the Working Group, who will almost certainly deem it unnecessary. Moreover, I wouldn't want to make any static analyzers throw any new warnings when downstream users have done nothing but update their headers (e.g. think about casts to the parameter type).
I think the impact of saying "the group attribute means that parameter, whatever type it may be, accepts enums from this group" is far less than saying "the group attribute means that parameter must be a GLenum
parameter and accepts enums from this group".
Finally, I don't think augmentation of the XML specification for downstream consumers of it should impact the headers in any way. Ultimately, all of this is irrelevant to Khronos' needs and if we do want to augment the specification with extra data, it should be done in a way that does not impact Khronos.
the group attribute means that parameter, whatever type it may be, accepts enums from this group
I'm fine with this.
So remove if I've got the sentiment right:
group
tags that are not referring to enums.GLenum
, which would mean, "this parameter takes all the values from the enum group, but also possibly some other values".Sounds good to me!
There is a lot of different parameters that are marked with
group
attributes that are not actually enum groups, but some other kind of metadata. Some examples includes:These do not denote enum groups like the majority of groups, but instead indicate some other out-of-band information about these parameters.
In total I'm pretty sure that atm there are 1076 functions, with a total of 1849 parameters that are tagged with an group but doesn't have the
GLenum
type.Unfortunately some of these parameters are actually enums, but their type doesn't reflect this. See #516 for example. There are 20 functions that fit this description: (written as
<type> <parameter name>: <group>
)There are also a lot of places where parameters of type
GLboolean
are marked with the groupBoolean
, this is technically a valid group as there are entries inside of it, but it would make no sense to changeGLboolean
toGLenum
(and it would be breaking too).It would be nice if something could be done about this.
I would like to propose the following course of action.
GLenum
instead of their currentGLint
/GLuint
(uint
might be a problem here).group
to something likekind
.group
attributes indicate valid enums, and this way we can work towards complete correctness for enum groups.This is working towards having robust enum groups that were discussed in #481, this can be used as somewhat of a discussion forum for figuring out how these changes should be done.
Here I'm attaching a file containing a complete breakdown of the functions that either are valid enums but do not sure
GLenum
or have a group attribute that is not a valid enum (with theBoolean
group filtered out).misplaced_groups.txt