Skip to content

fix unsafe.Pointer use to be compatible with cgo rules #74

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

Closed
wants to merge 1 commit into from

Conversation

calebdoxsey
Copy link

According to golang/go#14210 it's necessary to take the address of a byte slice in the same expression as the cgo function invocation.

IE do this:

C.f(unsafe.Pointer(&dst[0]))

And not this:

tmp := unsafe.Pointer(&dst[0])
C.f(tmp)

This PR will fix #24 without resorting to CBytes, and therefore avoiding a copy.

@ghost
Copy link

ghost commented Jul 12, 2017

@confluentinc It looks like @calebdoxsey just signed our Contributor License Agreement. 👍

Always at your service,

clabot

@edenhill edenhill self-requested a review July 13, 2017 11:39
@edenhill
Copy link
Contributor

This is great stuff, will tend to this soon.
Thank you


if msg.Value != nil {
valLen = len(msg.Value)
// allow sending 0-length messages (as opposed to null messages)
Copy link
Contributor

@edenhill edenhill Aug 7, 2017

Choose a reason for hiding this comment

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

There is a difference between an empty Value/Key field (length 0) and a Null message (Value/Key is nil).
Empty messages are treated like any other message, just empty, while Null messages values are considered tombstones and null message keys are considered key-less.
That's the reason why the original code initialized both valp and keyp to nil (NULL in C) and I'm guessing this will break the cast-on-same-line requirement of Go.
One way around this might be to have four different code paths depending on the nullity of key and value, e.g.:

 if  key && val {
    produce(..., key, val, ..)
 else if key && !val {
   produce(..., key, C.NULL, ..)
 ..and so on

@edenhill
Copy link
Contributor

edenhill commented Aug 9, 2017

Fixed in #83

Thank you for your PR, much appreciated!

@edenhill edenhill closed this Aug 9, 2017
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.

json.Marshal results in "panic: runtime error: cgo argument has Go pointer to Go pointer"
2 participants