search

KhronosGroup / OpenGL-Registry

OpenGL, OpenGL ES, and OpenGL ES-SC API and Extension Registry
679 stars 274 forks source link

glPathGlyphIndexRangeNV has an *old*? syntax for constant size arrays '[2]' #455

Closed frederikja163 closed 3 years ago

frederikja163 commented 3 years ago

In the xml file glPathGlyphIndexRangeNV has a unique way of defining an array of constant length, Should this rather use the Len attribute? Or would such a change break anything on the C side, I know that it would be really handy for our bindings. Im happy to implement the change myself aswell.

Perksey commented 3 years ago

Unfortunately the C header generator doesn't pick up on len! So while it's inconsistent with the XML file, this'll likely have to stay now it's shipped.

frederikja163 commented 3 years ago

If that dosent pick up on len how does it manage the other constant length arrays?

Perksey commented 3 years ago

They're just pointers.

eddyb commented 3 years ago

So while it's inconsistent with the XML file, this'll likely have to stay now it's shipped.

I don't think T[2] vs T*can be a backwards-compatibility issue? It's not T[static 2] which means "array of length 2", it's T[2] which is just "array of T" and the 2 doesn't do anything.

eddyb commented 3 years ago

Even if it were T[static 2], looks like even that would be backwards-compatible: https://godbolt.org/z/jsz1Ma.

Perksey commented 3 years ago

It's not my call to make, but generally we avoid XML changes that result in changes to the resultant header.

Lokathor commented 3 years ago

This is one of the annoying one-off special cases that a program parsing the XML needs to know to look for and needs to know how to convert. Using the len tag attribute like other places to would be much better.

Perksey commented 3 years ago

For sure, but the XML file, as far as Khronos is concerned, is just a file used to organize commands and enums, as well as construct C headers - the latter being the most influential factor in its design. The function pointers and typedefs are a great example of this.

eddyb commented 3 years ago

Are changes to comment="..." attributes allowed? If so, how is this any different? The T x[2] syntax might as well be T *x /* 2 */.

Perksey commented 3 years ago

comment attributes are ignored by the header generator, so yes changes are allowed but it won't make any difference. You can add a len attribute in addition to the [2] syntax as this wouldn't affect the header.

Lokathor commented 3 years ago

Question: Why can't the header generator simply pay attention to the len attribute?

But also: the comment field should not be used for this, just the len field.

eddyb commented 3 years ago

But also: the comment field should not be used for this

Correct, I was making a comparison in terms of semantic impact, having misremembered that comments end up in the header.

Looks like long-term, gl.xml isn't as usable as it could be, and someone will probably have to start to maintain a fixed up version of it - I've been eyeing an automated merge with Mesa's gl_API.xml, which has extra information Khronos doesn't seem to be willing to put into gl.xml anyway.

Lokathor commented 3 years ago

i have approximately come to the same conclusion and have begun doing that in the past few weeks. though not in xml form, just directly in rust form in my case.

NogginBops commented 3 years ago

I mean it's all up to Khronos if they think this is a change they can make. So we have to wait to hear from them on this issue.

Semantically the c code is equivalent to a pointer argument (it will compile to the same thing), so it's not a correctness issue.

If anything it is most likely an issue related to documentation and extension specification consistency (it should ideally be updated in the NV_path_rendering extension too, but that might be hard).

Perksey commented 3 years ago

I think this is one that one of the NVIDIA guys should comment on too as this is their discrepancy, but I'm sure Piers will see this in time.

pdaniell-nv commented 3 years ago

It's not totally clear to me what the change related to NV_path_rendering you have in mind would be, but if its a valid fix I can help push that through. What does the XML change look like?

Perksey commented 3 years ago

As I understand it, the participants in this issue propose:

-<param><ptype>GLuint</ptype> <name>baseAndCount</name>[2]</param>
+<param len="2"><ptype>GLuint</ptype>* <name>baseAndCount</name></param>

The argument being that [2] is non-standard for the XML and headers.

pdaniell-nv commented 3 years ago

That change sounds good to me. If you want to create a PR for it I'll approve and get it merged. I'll also follow up with an extension spec tweak.

Perksey commented 3 years ago

Done, #462