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

cmd/cgo: documentation for C.GoString could be more complete #32734

Open
DryHumour opened this issue Jun 21, 2019 · 8 comments
Open

cmd/cgo: documentation for C.GoString could be more complete #32734

DryHumour opened this issue Jun 21, 2019 · 8 comments

Comments

@DryHumour
Copy link

@DryHumour DryHumour commented Jun 21, 2019

Documentation for C.GoString(), C.GoStringN(), and C.GoBytes() should probably explicitly document behaviour for "special" values (e.g. NULL pointer, negative integers).

https://golang.org/cmd/cgo/#hdr-Go_references_to_C

System details

go version go1.12.6 linux/amd64
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/nschelle/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/nschelle/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
GOROOT/bin/go version: go version go1.12.6 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.12.6
uname -sr: Linux 4.4.0-146-generic
Distributor ID:	Ubuntu
Description:	Ubuntu 16.04.6 LTS
Release:	16.04
Codename:	xenial
/gnu/store/h90vnqw0nwd0hhm1l5dgxsdrigddfmq4-glibc-2.28/lib/libc.so.6: GNU C Library (GNU libc) stable release version 2.28.
gdb --version: GNU gdb (GDB) 8.3
@andybons andybons added this to the Unplanned milestone Jun 25, 2019
@andybons

This comment has been minimized.

Copy link
Member

@andybons andybons commented Jun 25, 2019

@ianlancetaylor in case he has opinions

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 25, 2019

I'm not opposed to this. I don't think it's very important. For functions like these I don't think it's all that interesting to document precisely how they break. No working program is going to pass them invalid values.

@gwd

This comment has been minimized.

Copy link

@gwd gwd commented Jun 28, 2019

Well NULL is (or could be) a valid value. I'm at the moment reviewing some improvements to some golang bindings for a C library. A particular C library function takes a numeric ID for an entity and returns a string (which the caller must free) if the ID is found, or NULL if it's not found. I'm trying to figure out if the following code is correct:

func (Ctx *Context) DomidToName(id Domid) (name string) {
	cDomName := C.libxl_domid_to_name(Ctx.ctx, C.uint32_t(id))
	defer C.free(unsafe.Pointer(cDomName))

	name = C.GoString(cDomName)
	return
}

If cDomName is NULL, will C.GoString return nil? Or do I have to do the check myself? I haven't been able to find the answer so far.

Defining what will happen if C.GoString is passed NULL seems like a minimum requirement.

@gwd

This comment has been minimized.

Copy link

@gwd gwd commented Jun 28, 2019

Same with C.free really -- in normal C, calling free on NULL is fine; I'd assume it's the same in cgo, but it would be good for that to be documented somewhere.

@DryHumour

This comment has been minimized.

Copy link
Author

@DryHumour DryHumour commented Jun 28, 2019

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 28, 2019

c.GoString returns a string, so it never returns nil.

At present if you call c.GoString with nil it will return the empty string. I'm fine with documenting that.

C.free is not specially implemented by cgo. It will simply call the C free function. If the C free function on your system ignores NULL pointers (as it should), then C.free will ignore nil pointers.

@gwd

This comment has been minimized.

Copy link

@gwd gwd commented Jun 28, 2019

Awesome, thanks.

@gwd

This comment has been minimized.

Copy link

@gwd gwd commented Jun 28, 2019

@DryHumour No, getting things documented for backwards compatibility is always a good idea, rather than just relying on the implementation never to change. Things end up changing for all kinds of random reasons that people don't even notice. If there's documentation that GoString behaves in a certain way, then people will check to make sure that behavior is maintained; if not, they just might forget that's a possibility, and change the behavior without even realizing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.