KhronosGroup / OpenGL-Registry

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

glCreateShaderProgramvEXT "strings" parameter does not match glCreateShaderProgramv #545

Closed null77 closed 1 year ago

null77 commented 1 year ago

From gl.xml:

        <command>
            <proto class="program"><ptype>GLuint</ptype> <name>glCreateShaderProgramv</name></proto>
            <param group="ShaderType"><ptype>GLenum</ptype> <name>type</name></param>
            <param><ptype>GLsizei</ptype> <name>count</name></param>
            <param len="count">const <ptype>GLchar</ptype> *const*<name>strings</name></param>
        </command>
        <command>
            <proto class="program"><ptype>GLuint</ptype> <name>glCreateShaderProgramvEXT</name></proto>
            <param group="ShaderType"><ptype>GLenum</ptype> <name>type</name></param>
            <param><ptype>GLsizei</ptype> <name>count</name></param>
            <param len="count">const <ptype>GLchar</ptype> **<name>strings</name></param>
        </command>

Notably strings has conflicting types const GLchar *const* and const GLchar **. These are not compatible types in C-based languages so it would be good to resolve the type mismatch. Most likely the EXT has the incorrect annotation.

Perksey commented 1 year ago

Is it possible to resolve this issue without breaking users of either function?

NogginBops commented 1 year ago

It does look like the specification and EXT_separate_shader_objects have different definitions of this function.

4.6 spec:

uint CreateShaderProgramv( enum type, sizei count, const char * const *strings );

EXT_separate_shader_objects:

uint CreateShaderProgramvEXT(enum type, sizei count, const char **strings);

not sure when and where the difference appeared but it's where.

Making a change where would at least not be ABI breaking (at least to my knowledge). Is it a problem that they have different definitions?

My initial thought would be to leave things as they are as the two functions are ABI compatible, and leaving the assumptions the same as when driver support for the extension was being developed reduces the risk of some driver doing some weird modification even though the argument is specified as const. Though if there is a good solution or explanation that would be more optimal for sure.

EDIT: There seems to also be a separate extension with the same name EXT_separate_shader_objects which is a OpenGL extension while the other extension with the conflicting definition is an OpenGL ES extension. So there might not be any conflict here at all to begin with.

SunSerega commented 1 year ago

These are not compatible types in C-based languages

They are, but only as a one-way conversion:

    const char** s1 = nullptr;
    // OK: Only adding const
    const char* const* s2 = s1;
    // Error: Cannot implicitly remove const
    const char** s3 = s2;

So adding const can't break any uses of this function. And I don't think any implementation would have a reason to do strings[i] = some_unrelated_string;, so the const char* const* should be more reflective of what's happening.

Perksey commented 1 year ago

We likely can't make assumptions about that, because if we do someone somewhere will inevitably be doing that and will be broken the second they update their headers ;)

NogginBops commented 1 year ago

What SunSerega is saying is that the update to the headers cannot source break any project. It's adding const to the parameter and will be compatible with any call to the function. The question is if any drivers that use the fact that the parameter is non-const.

pdaniell-nv commented 1 year ago

FWIW, this change won't break NVIDIA drivers and I think we should accept the fix. I'll check with other IHVs in the next OpenGL/ES meeting.

pdaniell-nv commented 1 year ago

Discussed in the OpenGL/ES joint WG meeting today and there was no objection to this change even if it does require a minor tweak to the driver build when members pull in the next header.