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

Fix GLsync definition #85

Merged
merged 2 commits into from
Feb 18, 2018
Merged

Fix GLsync definition #85

merged 2 commits into from
Feb 18, 2018

Conversation

errcw
Copy link
Member

@errcw errcw commented Jun 4, 2017

Even though GLsync is defined as an opaque struct pointer, some drivers
use non-pointer values for GLsync, which is incompatible with Go pointer
semantics. As a workaround we redefine GLsync to be a pointer-width
numeric value.

Even though GLsync is defined as an opaque struct pointer, some drivers
use non-pointer values for GLsync, which is incompatible with Go pointer
semantics. As a workaround we redefine GLsync to be a pointer-width
numeric value.
@dmitshur
Copy link
Member

dmitshur commented Jun 5, 2017

@errcw, sorry, I won't be able to review because of https://twitter.com/shurcool/status/871528913108971520. Maybe ask @dominikh?

@dmitshur dmitshur removed their request for review June 5, 2017 02:16
type.go Outdated
@@ -106,7 +106,9 @@ func (t Type) GoType() string {
// an integer type.
return t.pointers() + "uintptr"
case "GLsync":
return t.pointers() + "unsafe.Pointer"
// GLsync is treated as an opaque, pointer-width type. Additional special
// case handling is required for the corresponding typedef, see below.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not very clear to me what "below" refers to. Do you mean CTypedef? Then it'd be better to say "see CTypedef."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@dmitshur
Copy link
Member

As far as code goes, I've posted one minor comment above, otherwise LGTM.

In terms of functionality, this PR is meant to resolve #71, is that so? If so, it should be verified/tested by those who can reproduce #71.

I'm not spotting any problems, but I can't say I have a full understanding of the problem details.

@dmitshur
Copy link
Member

@errcw What's the status of this PR?

@errcw
Copy link
Member Author

errcw commented Sep 20, 2017

Pending verification from dominikh@ in go-gl/gl#71 that this PR resolves the issue.

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

Successfully merging this pull request may close these issues.

2 participants