Repository navigation
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
This PR removes a duplicate const segmentedHeap = true from the embedded Go source fixture gcSegmentWorkloadSource, which previously collided with var segmentedHeap = true on line 83. The fix is correct and minimal.
The var form is the right one to keep: the fixture reassigns segmentedHeap at resetHeap() (line 94, sets true) and TestSingleArenaLookup (line 209, sets false) so a single test binary can exercise both the segmented and single-arena lookup paths. A const could neither coexist with the var nor be reassigned, so the base revision's generated fixture would fail to compile — this deletion resolves that.
All four review passes (code quality, performance, security, documentation accuracy) found no blocking issues. One optional maintainability note below; nearby comments remain accurate.
Note (out of scope for this one-line fix): because the entire test package is embedded as a backtick string, the compiler cannot validate it until the outer test spawns go test, which is how this duplicate declaration went unnoticed. If this fixture keeps growing, consider moving the source into a testdata/ file so editors and gofmt/go vet can validate it.
Additional findings
internal/build/wasm_gc_segments_test.go:83: [P3] Document why the fixture uses var, not the runtime's const: Optional, non-blocking. In the real runtime,segmentedHeapis a build-tag-selectedconst(falseinruntime/internal/runtime/tinygogc/gc_segment_default.go,trueingc_segment_wasi_threads.go). This fixture deliberately makes it a mutablevarso one test binary can toggle both lookup paths (TestSingleArenaLookupsets it tofalse). A short comment here — e.g.// const in the runtime; a var in the fixture so tests can toggle both lookup paths— would document that intentional divergence and discourage a future reader from "restoring" theconstthat this PR just removed.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The same correction is now merged into main through #2821. No additional code change is needed.
TestWasmGCSegmentBoundariesAndSweepgenerates a fixture that declaressegmentedHeapas both a constant and a variable. Go CI fails before running the metadata checks because the name is redeclared and the test cannot switch heap modes.Remove the redundant constant and retain the existing variable used by the segmented and single-arena cases.
Validation: reproduced the compile failure before the change; the segment, head-cache, and arena fixtures all pass afterward with
go test -count=1 -timeout=90s -run '^TestWasmGC' wasm_gc_segments_test.go wasm_gc_head_cache_test.go wasm_gc_arena_test.gofrominternal/build.