Skip to content

Create example of debug useable dcheck equivalent for Go. #141

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.

Sign up for GitHub

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 2 commits into from
Closed

Create example of debug useable dcheck equivalent for Go. #141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a504095
Create example of debug useable dcheck equivalent for Go.
rsned Apr 3, 2025
70b7551
add comments
rsned Apr 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions s2/debug_disabled.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Copyright 2025 Google Inc. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

//go:build !debug

package s2

// dcheck is a no-op in non-debug builds.
func dcheck(condition bool, message string) {
// no-op
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move the discussion of what to actually do back to #104

}
35 changes: 35 additions & 0 deletions s2/debug_enabled.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Copyright 2025 Google Inc. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

// To use these, you must set the build tag with the -tags flag as:
// go build -tags debug

//go:build debug

package s2

// dcheck is a custom debug check function.
func dcheck(condition bool, message string) {
if !condition {
panic("dcheck failed: " + message)
}
}

// TODO(rsned): Also create equivalents for
// ABSL_DCHECK_EQ
// ABSL_DCHECK_GE
// ABSL_DCHECK_GT
// ABSL_DCHECK_LE
// ABSL_DCHECK_LT
// ABSL_DCHECK_NE