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

[xml] glGetString commands use "GLubyte*" instead of "GLchar*" for strings #363

Closed
SunSerega opened this issue Jan 30, 2020 · 10 comments
Closed
Assignees
Milestone

Comments

@SunSerega
Copy link
Contributor

        <command>
            <proto group="String">const <ptype>GLubyte</ptype> *<name>glGetString</name></proto>
            <param group="StringName"><ptype>GLenum</ptype> <name>name</name></param>
            <glx type="single" opcode="129"/>
        </command>
        <command>
            <proto group="String">const <ptype>GLubyte</ptype> *<name>glGetStringi</name></proto>
            <param group="StringName"><ptype>GLenum</ptype> <name>name</name></param>
            <param><ptype>GLuint</ptype> <name>index</name></param>
            <glx type="single" opcode="214"/>
        </command>

(these are only 2 functions, using GLubyte* as string)

Type GLchar is already defined and used by some extensions.
And it makes much more sense in this context, then GLubyte with group="String".

@SunSerega SunSerega changed the title [xml] glGetString functions use "GLubyte*" instead of "GLchar*" for strings [xml] glGetString commands use "GLubyte*" instead of "GLchar*" for strings Jan 30, 2020
@oddhack
Copy link
Collaborator

oddhack commented Feb 3, 2020

The group will consider this, though it's unlikely we're going to change an actual API signature.

@oddhack oddhack added this to the Needs Triage milestone Feb 3, 2020
@SunSerega
Copy link
Contributor Author

Can you explain in more details, what can be bad about changing signature in this way?

GLchar and GLubyte are synonyms in C++. And i think most of people are using char* as a type of strings they pass to API anyway. So this shouldn't break any existing C++ code.

And, as i said, there are other commands in .xml's, that are using GLchar* for string parameters. So this shouldn't break any custom .xml parser's.

And while only visually - it is wrong. This is like using (int)NULL in math - it works, but you shouldn't ever do that.

@Perksey
Copy link
Contributor

Perksey commented Feb 3, 2020

Changing the signature will be a breaking change, while a beneficial one, a breaking change nonetheless. As you said, GLchar is the same as GLubyte, so all the group is likely to do is better define this as the case in the spec.

Chipping in my own opinion here, I do agree that GLchar is desirable but a breaking change is not desirable, but I do think the group should either use GLchar* or GLubyte*, not both, to ensure that strings are consistently defined.

@SunSerega
Copy link
Contributor Author

Changing the signature will be a breaking change

Well it wouldn't be breaking but...

Just checked - they aren't actually same, GLubyte is unsigned char. And so this will not work without pointer cast:

GLubyte * ptr1 = NULL;
GLchar * ptr2;
ptr2 = ptr1;

Well, in that case i think it's understandable to not want to fix consistency. But, in that case there should be comment in .xml and something about this in spec.

As i said in first comment here - glGetString and glGetStringi are the only commands with GLubyte used for string, so there is only one place in .xml and spec where this should be specified.

@Perksey
Copy link
Contributor

Perksey commented Feb 3, 2020

Gotcha. So all that's needed is some clarification I think and then this issue is resolved.

@pdaniell-nv
Copy link
Contributor

It's not clear to me where the proposed clarification should go. In the XML? @SunSerega do you have a PR in mind?

@SunSerega
Copy link
Contributor Author

Clarification should explain that while prototype shows that glGetString and glGetStringi return GLubyte* - this is an old typo, and they actually return GLchar*.

And no, i haven't yet thought about exact words for PR.

@oddhack
Copy link
Collaborator

oddhack commented Feb 11, 2020

This is a spec issue, not XML. The utility of noting that there's something peculiar about the return type of a nearly 30 (?!) year old API seems minimal to me, but we could put in a footnote.

@pdaniell-nv
Copy link
Contributor

We discussed this in the OpenGL/ES working group meeting today. I learnt an interesting bit of history that gives some explanation as to why these functions have been specified this way. Way back in OpenGL 1.x days there were no functions which take a string from the application as input to the OpenGL implementation. For returning a string from OpenGL to the application there is "GLubyte * glGetString()", and in that case the OpenGL specifies how it should be interpreted so it could use GLubyte.

Then came along GL_ARB_shader_objects, which added a way for the application to pass a string into OpenGL. If you see issue 6 you'll note that it was a conscious decision to add the new GLcharARB type. Now that strings are passed from the application to OpenGL it made sense for OpenGL to accept standard C/C++ strings as is, without trying to reinterpret them as GLubyte. Thus GLchar was born and added to OpenGL 2.0.

And for the reasons suggested above, the old glGetString() functions were not changed so that they remained strictly backwards compatible. So the fact glGetString returns GLubyte is not a typo, just an artifact of history.

I suggest we don't change the XML or spec. Maybe just add a comment to the XML pointing to this issue.

@Perksey
Copy link
Contributor

Perksey commented Apr 24, 2020

I suggest we don't change the XML or spec. Maybe just add a comment to the XML pointing to this issue.

See #396 which closes this issue.

@oddhack oddhack closed this as completed in 1637f31 May 7, 2020
oddhack added a commit that referenced this issue May 7, 2020
heisluft added a commit to heisluft/Beef-OpenGL-Header-Generator that referenced this issue Jul 13, 2021
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

No branches or pull requests

4 participants