Create example of debug useable dcheck equivalent for Go.

[rsned]

No description provided.

[jmr]

Is this going to get inlined and have zero overhead? Is this a common go pattern?

[rsned]

Using build tags is a common thing in go. We already use them in

bits_go18.go://go:build !go1.9
bits_go18.go:// +build !go1.9
bits_go19.go://go:build go1.9
bits_go19.go:// +build go1.9

Using a dcheck in go? No idea, but I suspect a lot of people have written their own variation as needed to add something like it to their go.

One sort of option is to kind of use is https://pkg.go.dev/testing#Testing but that only works when you are running in unit tests which is not the same as a DCHECK

The compiler seems to strip the function out when it's the no-op version

To test and see I added a dcheck(cellid > 0) in cellid.String() and then called it with both to see.

a.go:

package main

import (
"fmt"

"github.com/golang/geo/s2"

)

func main() {
fmt.Printf("CellID x: %v\n", s2.CellID(123))
}

$ go build a.go
$ go tool objdump -S a | egrep dcheck

$ go build -tags debug a.go
$ go tool objdump -S a | egrep dcheck
dcheck(ci > 0, "holy moly!")

$ go tool objdump -S a | egrep -A5 -B2 dcheck
0x489bc7 4889e5 MOVQ SP, BP
0x489bca 4883ec28 SUBQ $0x28, SP
dcheck(ci > 0, "holy moly!")
0x489bce 4885c0 TESTQ AX, AX
if condition {
0x489bd1 772b JA 0x489bfe

objdump shows the call not existing in the non-debug version.

[flwyd]

Using a dcheck in go? No idea, but I suspect a lot of people have written their own variation as needed to add something like it to their go.

I don't think it is. I couldn't find any equivalent examples named dcheck in sourcegraph.com/search, nor any hits for lang:go go:build\s+debug panic (likewise with the test tag). Additionally, I don't think go test automatically sets any tags, so users would (a) need to know they can turn this on, (b) turn it on in their test environments, and (c) have tests which ensure their production code won't hit the issue the library is dcheck'ing. (That is, there's no "if things are running on Forge we'll probably catch any bugs with dcheck.")

As I understand it, the C++ idea behind DCHECK is that the check would be expensive to compute, so the macro removes the code which performs the check, as well as the induced error, in production builds. To get equivalent optimization in Go to what DCHECK provides in C++, you'd need to get rid of the call to computeExpensiveBoolean() in dcheck(computeExpensiveBoolean(), "oh crap"), not just the call to if !condition { panic } in the debug version of the check call.

Go style generally prefers errors that are either made explicit or defined out of existence. The error highlighted in #104 is a condition on the argument to the function. We could change the return type to ([]CellID, error) and force the caller to deal with the possibility they passed an invalid value for level. Or we could return some sensible value if the parameter is invalid, e.g. clamping level to max(level, ci.Level()) or returning an empty slice if level is invalid. panic would be less desirable (it's not generally a stand-in for Java's IllegalArgumentException), but we could consider a style of offering two function variants: Foo() returns (T, error) and MustFoo() returns T or panics and it's the caller's responsibility to know the panic isn't possible. regexp.Compile/regexp.MustCompile are a good example of this pattern.

Error handling is an area where there's some tension between keeping S2 code consistent between languages and providing users an ergonomic API in their language of choice. This shows up some in the Java API which borrows the S2Error object from C++ in some places that would feel more idiomatic to throw an exception, but C++ S2 doesn't use exceptions. It would be valuable to make a broad style decision about error handling for Go S2 and in what ways it may deviate from the C++ API.

[jmr]

As I understand it, the C++ idea behind DCHECK is that the check would be expensive to compute, so the macro removes the code which performs the check, as well as the induced error, in production builds.

Yes, I agree. It's assert() with better error messages.

IMHO, we should do what's idiomatic for the language, so Foo/MustFoo. We can omit MustFoo if the performance difference isn't expected to be meaningful.

[jmr]

Go style generally prefers errors that are either made explicit or defined out of existence.

In this case, we could easily do this by defining AllNeighbors to be the neighbors of the parent at that level if the level previously would have been out of range. The common case seems to be c.AllNeighbors(c.level). Not sure if that's worth an extra function/rename.