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

runtime: unusual code in proc.go could use documentation #20717

Closed
velovix opened this issue Jun 18, 2017 · 4 comments

Comments

Projects
None yet
4 participants
@velovix
Copy link

commented Jun 18, 2017

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

go version devel +d293c85e39 Sat Jun 17 08:17:14 2017 +0000 linux/amd64

What operating system and processor architecture are you using (go env)?

GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/velovix/go"
GORACE=""
GOROOT="/home/velovix/go-source"
GOTOOLDIR="/home/velovix/go-source/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build407292918=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

Please see this discussion on golang-nuts:
https://groups.google.com/d/topic/golang-nuts/FnF3oZeJ7aY/discussion

There is some code at lines 198-202 in /src/runtime/proc.go that could do with a bit of documentation. The code in question can be found here on Github.

Here is my current draft:

// In case exit fails to stop the program, try to stop it by causing a
// panic. If that doesn't work, stall the program.
// This is here to help catch problems with new ports, where exit may not
// be properly implemented.

I will send in a CL if that looks okay.

@ALTree ALTree changed the title Unusual code in proc.go could use documentation runtime: unusual code in proc.go could use documentation Jun 18, 2017

@ALTree

This comment has been minimized.

Copy link
Member

commented Jun 18, 2017

I will send in a CL if that looks okay.

Just sending the CL is fine: it will be reviewed (and commented on) on gerrit. Remember to link this issue in the commit message.

@gopherbot

This comment has been minimized.

Copy link

commented Jun 21, 2017

CL https://golang.org/cl/46212 mentions this issue.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2017

Comment I added to CL:

_I've changed my mind. This doesn't need careful documentation. It's a common pattern for code in runtimes - and other systems - to cause deliberate crashes when things are clearly not working.

To someone reading the code, it should be very clear that this code will cause a trap, and if the reader is not accustomed to reading runtimes, a glance to the line immediately preceding should make it very clear why we're trapping._

If you follow this to its conclusion, you'll end up needing to comment everything about how everything works for people unfamiliar with such systems. That's not a good precedent.

@velovix

This comment has been minimized.

Copy link
Author

commented Jun 22, 2017

I did not realize that this snippet existed more than once in the runtime. If it's a pattern, I can see why we wouldn't want a long comment block explaining it.

@velovix velovix closed this Jun 22, 2017

@golang golang locked and limited conversation to collaborators Jun 22, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.