My Failed Go PR: Lessons on Benchmark Overflow, Testing, and Commit Message Practices

I previously discovered a Go benchmark timeout bug (see my earlier article "I Think I Found a Go Bug?") and submitted a pull request (PR) to fix it, hoping it would be merged and I could claim contributor status.

First Submission

Go uses Gerrit for code review, not GitHub, so my PR was mirrored from a GitHub issue. The issue was initially closed as a duplicate, but after some discussion I convinced a reviewer that my code needed a fix.

The problematic code snippet was:

func (b *B) launch() {
    ...
    // n (int64) may overflow
    n = goalns * prevIters / prevns
    ...
}

The reviewer pointed out that my overflow check was unsafe and suggested adding proper tests.

Overflow Consideration Incomplete

The reviewer showed a demonstration where positive overflow becomes negative and then positive again, emphasizing the need for robust overflow handling.

Need Tests

He also stressed that the code must include tests. I added a unit test that limits the benchmark to 150 seconds and aborts after six attempts.

Second Submission

In the second PR I replaced the previous overflow check with a reverse‑calculation method, similar to a technique I used in a university C‑language course, and added a unit test.

The test sets a 150 s timeout, increments a counter on each probe, and aborts if the counter exceeds six, indicating a problem.

Commit Message Issues

The reviewer noted that my commit message was non‑standard. Go’s commit‑message guidelines require a short summary line, a blank line, and a detailed description without markup, plus a reference to the associated issue using Fixes #12345 or similar.

math: improve Sin, Cos and Tan precision for very large arguments The existing implementation has poor numerical properties for large arguments, so use the McGillicutty algorithm to improve accuracy above 1e10. The algorithm is described at https://wikipedia.org/wiki/McGillicutty_Algorithm Fixes #159

Testing Concerns

The reviewer also highlighted that I was calling unexported functions and variables directly in the benchmark package and suggested using flag.Lookup or integration tests via cmd/go/testdata/script instead.

Lack of Comments

Missing comments made the code hard to understand. The reviewer asked why the benchmark would really run for 150 seconds and how the limit of six attempts was determined.

Re‑thinking Overflow

Instead of checking n for overflow, it may be better to verify whether goalns exceeds int64_max * prevIters , or to perform calculations using float64 for a larger range.

Conclusion

Although the PR was not merged, I learned valuable lessons about Go’s review process, proper overflow handling, testing strategies, and commit‑message standards, which I plan to apply in future contributions.