chore: enable the modernize linter for idiomatic packages - #88
Conversation
The Go 1.27 and golangci-lint v2.13.2 bump resolved the v2 panic on atomic types that had kept modernize disabled. Enable it, scoped so the transliteration packages (internal/celt, internal/silk, ...) and the cgo differential harness (internal/reftest) keep their C-mirroring loops: modernize's rangeint and loop rewrites fight diffability against the libopus reference there, the same reason those packages already relax the other style linters. Apply the four resulting suggestions in the idiomatic packages: max/min builtins in the oggopus decoder and writer, and a range-over-int loop plus a min() in two test helpers. All four are behavior-preserving and covered by existing tests.
Code Review ✅ ApprovedEnables the modernize linter for idiomatic packages now that Go 1.27 and golangci-lint v2.13.2 resolve the atomic types panic. Applies four modernize suggestions: OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (5)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. WalkthroughThe change enables the ChangesModernize lint adoption
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~5 minutes Merge Risk: ⚪ Minimal · up to This enables modernize linting with exclusions for reference-mirroring code and replaces several equivalent expressions with Go built-ins. The changes are behavior-preserving and ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🟡 Changes recommended
There is a potential slice-bounds panic on 32-bit platforms due to converting an int64 delta to int before clamping in oggopus/decoder.go.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Enables the modernize golangci-lint linter for idiomatic packages (while keeping transliteration/C-mirroring packages excluded) and applies the resulting behavior-preserving refactors (built-in min/max and range-over-int loops).
Changes:
- Enable
modernizein.golangci.yaml, with explicit exclusions for transliteration packages andinternal/reftest. - Refactor selected loops and min/max patterns in
oggopusand tests to modern Go idioms (min/max,for i := range n,for x := range uint8(4)).
File summaries
| File | Description |
|---|---|
| oggopus/writer.go | Uses max() to simplify final granule monotonicity guard. |
| oggopus/family1_test.go | Uses min() and range-over-int for prefix-diff helper. |
| oggopus/decoder.go | Uses max() to simplify end-trim clamping logic. |
| internal/packet/toc_test.go | Uses range-over-int for compact frame-count sweep loop. |
| .golangci.yaml | Enables modernize and excludes it where C-diffability is required. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if d.limit >= 0 && d.delivered+int64(perChan) > d.limit { | ||
| keep := int(d.limit - d.delivered) | ||
| if keep < 0 { | ||
| keep = 0 | ||
| } | ||
| keep := max(int(d.limit-d.delivered), 0) | ||
| samples = samples[:keep*d.info.Channels] |
There was a problem hiding this comment.
Good eye, but it's safe as written and this change is behavior-preserving: the old keep := int(d.limit-d.delivered); if keep < 0 { keep = 0 } and the new keep := max(int(d.limit-d.delivered), 0) do the int64 to int conversion in exactly the same place, so nothing about the conversion changed here.
At this point d.limit - d.delivered is bounded to [0, perChan). The clamp only runs under d.limit >= 0 && d.delivered+perChan > d.limit, and delivered never exceeds limit (once the end-trim fires, delivered is pinned to limit). perChan is a single Opus frame (<= 5760 samples per channel), so the value fits int32 on every platform, and the max(..., 0) floors the late-discovered-limit case to 0. The only way to feed a large-magnitude int64 into that conversion is a limit discovered late after roughly 2 billion over-delivered samples on a 32-bit build, which is pre-existing and orthogonal to enabling a linter.
I'll track hardening the clamp to int64 (and capping to perChan) as a separate follow-up rather than fold an untrusted-input logic change into this PR.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Summary
The Go 1.27 and golangci-lint v2.13.2 upgrade resolved the golangci-lint v2 panic on atomic types that had kept the
modernizelinter disabled, so this enables it for the idiomatic packages. It is scoped to skip the deliberately C-shaped transliteration packages (internal/celt,internal/silk,internal/rangecoding,internal/fixedmath,internal/silkmath,internal/opuscompare,internal/opusdec,internal/opusenc) and the cgo differential harness (internal/reftest), because modernize'srangeintand loop rewrites fight line-by-line diffability against the libopus v1.6.1 reference those packages mirror. That matches how the other style and complexity linters are already relaxed in those same blocks.Applying the four suggestions modernize produced in the idiomatic packages:
max/minbuiltins in the oggopus decoder and writer, plus a range-over-int loop and amin()in two test helpers. All four are behavior-preserving and covered by existing tests.Test Plan
go build ./...,go vet ./..., andgofmt -lare cleangolangci-lint run ./...(v2.13.2, default build tags, the CI configuration) reports 0 issuesgolangci-lint run --enable-only=modernize ./...reports 0 issues, confirming every finding was applied and the excluded packages are suppressedgo test ./...passesSummary by CodeRabbit
Refactor
Tests