Address Copilot review findings from PR #2#3
Conversation
- generator: honor configured indentionSize in NewContext instead of hard-coding NewIndent(4). - types: return wrapped error from LoadTypes on parse failure rather than panicking, and drop stdout 'Found Type'/'Package'/'File' prints that made the CLI and tests noisy. - tests/generator_test.go: walk parents with filepath.Dir instead of splitting on '/' so the test-dir search is portable. - tests/var_test.go / tests/indent_test.go: remove stdout prints and add real assertions (including the previously assertion-less TestVarNamesIgnoreAllButOne, TestVarAlphaStrings, and TestBasicIndent). - util/bufferpool: guard isPowerOfTwo against 0/negative inputs, and rebind the pool size in newBufferPool so each sync.Pool.New closure captures its own size rather than the loop variable. Extend tests accordingly. - cmd/bingen: validate -version range before casting to uint8 so out-of-range values fail loudly instead of silently wrapping. - stringutil: store LoadOrStoreFunc values under the provided key (not under the computed value) and add tests covering the behavior. - generator/vars: panic with a clear message instead of looping forever when the skip set contains every alpha rune. Add vars tests covering the panic and the basic sequence. Co-authored-by: Alex Meijer <ameijer@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Addresses follow-up correctness and test gaps from the initial bingen contribution (PR #2), fixing several logic/behavior issues and strengthening unit tests around the affected code paths.
Changes:
- Fix generator/context configuration usage (indent size), buffer pool correctness (power-of-two check and pool slot allocation), string banking behavior (store under correct key), and var-name generation edge case (panic instead of infinite loop).
- Improve CLI/type loading behavior by removing noisy stdout prints and returning parse errors instead of panicking; add input validation for
-version. - Expand/strengthen tests across generator, vars, indent, bufferpool, and stringutil (including new test files).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
cmd/bingen/bingen.go |
Validates -version before casting to uint8. |
pkg/generator/context.go |
Honors configured indentation size in NewContext. |
pkg/generator/vars/vars.go |
Prevents infinite loop by panicking when skip set contains all letters. |
pkg/generator/vars/vars_test.go |
Adds coverage for panic behavior and core var-name helpers. |
pkg/types/types.go |
Removes debug prints; returns parse errors instead of panicking. |
pkg/util/bufferpool.go |
Fixes closure capture in pool initialization; corrects isPowerOfTwo for zero. |
pkg/util/bufferpool_test.go |
Extends isPowerOfTwo tests; adds regression test for pool slot allocation sizes. |
pkg/util/stringutil/stringutil.go |
Fixes LoadOrStoreFunc to store computed values under the provided key. |
pkg/util/stringutil/stringutil_test.go |
New tests covering string bank behavior and concurrent access. |
tests/generator_test.go |
Makes test directory discovery portable across OSes. |
tests/indent_test.go |
Strengthens indentation assertions; adds custom indent-size test. |
tests/var_test.go |
Removes stdout spam and adds deterministic assertions for var generation/mapping. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestBankDeduplicatesStrings(t *testing.T) { | ||
| ClearBank() | ||
| a := Bank("hello") | ||
| b := Bank("hello") | ||
| if a != b { | ||
| t.Errorf("expected Bank to return equal strings, got %q and %q", a, b) | ||
| } | ||
| } |
There was a problem hiding this comment.
TestBankDeduplicatesStrings only checks string equality (content), which will always pass even if Bank doesn't actually deduplicate allocations (and with string literals the compiler may already reuse the same backing data). This makes the test ineffective at guarding the intended behavior. Consider using dynamically constructed strings and asserting pointer identity via unsafe.StringData (or an equivalent) to verify that repeated banking returns the same underlying string data.
| if *version > 255 { | ||
| fmt.Fprintf(os.Stderr, "invalid -version value %d: must be between 0 and 255\n", *version) | ||
| os.Exit(2) |
There was a problem hiding this comment.
The 255 upper bound is correct for a uint8 cast, but using a named constant like math.MaxUint8 (or a local const maxVersion = 255) would make the intent clearer and avoid a magic number in the CLI validation.
| // Walk up parent directories looking for a "tests" sibling directory. | ||
| current := wd | ||
| for { | ||
| candidate := filepath.Join(current, "tests") | ||
| if dirExists(candidate) { |
There was a problem hiding this comment.
The comment says "tests" is searched as a sibling directory, but the code checks filepath.Join(current, "tests"), i.e. a child of each ancestor directory. Consider rewording the comment to match the actual search strategy (walking up ancestors and checking for a tests subdirectory).
Summary
Addresses the 14 Copilot review findings from PR #2. These were flagged on the initial bingen contribution but left as follow-ups; this PR cleans them up and adds test coverage for the affected code paths.
Changes
Correctness fixes
pkg/generator/context.go:NewContextnow uses the configuredgcf.indentionSizeinstead of hard-codingNewIndent(4), soNewGeneratorContextFactory'sindentionSizeparameter is actually honored.pkg/util/bufferpool.go:isPowerOfTwonow requirescapacity > 0. The old bit trick returnedtruefor zero, which was invisible behind thePutcall-site guard but wrong for any reuse.newBufferPool'ssync.Pool.Newclosures now capture a per-iterationsizevariable so each pool slot allocates buffers of the correct size. Under Go's pre-1.22 loop-variable semantics this would have caused every pool to hand back buffers of the final size; rebinding is still safer and more explicit.pkg/util/stringutil/stringutil.go:LoadOrStoreFuncnow stores the computed value under the providedkey(previously it stored undervalue, breaking the method contract whenkey != f()).pkg/generator/vars/vars.go:alphaVarNames.skip()now panics with a clear message if the skip set contains every alpha rune, instead of looping forever.Error handling & noise
pkg/types/types.go:LoadTypesreturns a wrapped error instead ofpanic-ing on parse failures, so the CLI and tests can report the problem gracefully.fmt.Println("Found Type", ...),fmt.Printf("Package: ..."), andfmt.Printf("File: ...")prints that spammed stdout during every CLI invocation and test run.cmd/bingen/bingen.go:-versionvalues greater than 255 now fail with a clear error before casting touint8, rather than silently wrapping.Test improvements
tests/generator_test.go: replacedstrings.Split(wd, "/")with portablefilepath.Dirtraversal so the test-dir search works on Windows.tests/var_test.go: removedfmt.Printlnspam inside hot loops;TestVarNamesIgnoreAllButOneandTestVarAlphaStringsnow make real assertions about the generated sequence and rune-to-char mapping.tests/indent_test.go:TestBasicIndentnow asserts the exact indentation string at each step (and a newTestIndentCustomSizeconfirmsNewIndent(n)honors a non-default size).pkg/util/bufferpool_test.go: extendedTestIsPowerOfTwoto cover0and negatives, and addedTestNewBufferPoolSlotsAllocateCorrectSizeto guard the closure-capture fix.pkg/util/stringutil/stringutil_test.go(new): coversLoadOrStore,LoadOrStoreFunc(including the regression where the value was stored under the wrong key),Clear,Bank,BankFunc, and concurrent access.pkg/generator/vars/vars_test.go(new): verifies the new panic behavior for a fully-populated skip set, the basic sequence,SkipAllBut,AlphaCharFor, and the "skip all but one letter" path.Verification
go build ./...go vet ./...go test ./...go test -race ./...All green.