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

syscall: improve NewCallback documentation and panic message #26138

Closed
aclements opened this issue Jun 29, 2018 · 5 comments
Closed

syscall: improve NewCallback documentation and panic message #26138

aclements opened this issue Jun 29, 2018 · 5 comments

Comments

@aclements
Copy link
Member

@aclements aclements commented Jun 29, 2018

What version of Go are you using (go version)?

Tip (commit 65d55a1)

Does this issue reproduce with the latest release?

Yes.

syscall.NewCallback (and syscall.NewCallbackCDecl) don't describe what's expected of the type of their argument beyond "it must be a function". But they do have requirements. For example, if you pass a function with no argument and no results, they panic with "compileCallback: function must have one output parameter". I don't actually know what this means a priori, and the NewCallback documentation doesn't tell me. The term "output parameter" doesn't appear in the Go spec, and it could (and does in this case) mean a result value, or it could mean a C-style output parameter which is actually an input argument that points to where to put the result.

We should document NewCallback*'s requirements and clarify the panic message for when they're violated.

/cc @alexbrainman

@aclements
Copy link
Member Author

@aclements aclements commented Jun 29, 2018

Ah, "output parameter" appears in the reflect documentation, which is probably where this terminology came from. That may be a bug in the reflect package documentation. :)

@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Jun 29, 2018
@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Jul 1, 2018

@aclements I totally agree. syscall.NewCallback documentation is bad.

syscall.NewCallback was used to allow for calling WindowProc ( https://msdn.microsoft.com/en-us/library/windows/desktop/ms633573(v=vs.85).aspx ) and service handler ( https://docs.microsoft.com/en-us/windows/desktop/api/winsvc/nc-winsvc-lphandler_function_ex ).

Alex

@jeet-parekh
Copy link
Contributor

@jeet-parekh jeet-parekh commented Jul 24, 2018

I could help with this. The documentation has to be updated in x/sys or syscall?

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Jul 24, 2018

I could help with this.

Please, do.

The documentation has to be updated in x/sys or syscall?

Both. I suggest you create CL for syscall first. And then we could copy it to x/sys.

Thank you.

Alex

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 26, 2018

Change https://golang.org/cl/126035 mentions this issue: syscall: improve NewCallback documentation and panic message

@gopherbot gopherbot closed this in 05f9b36 Jul 27, 2018
@golang golang locked and limited conversation to collaborators Jul 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.