Skip to content

fix: make uint32 saturation portable - #6

Merged
kolkov merged 3 commits into
gogpu:mainfrom
besmpl:agent/fix-386-saturation
Aug 12, 2026
Merged

fix: make uint32 saturation portable#6
kolkov merged 3 commits into
gogpu:mainfrom
besmpl:agent/fix-386-saturation

Conversation

@besmpl

@besmpl besmpl commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • replace the architecture-dependent min/max expression in saturateUint32 with explicit signed and unsigned bounds checks
  • preserve the existing clamp-to-math.MaxUint32 behavior on 64-bit systems
  • add portable boundary coverage that adapts to the host int width
  • keep the current golangci-lint output ordering configuration valid with the latest v2 parser

Why

The current expression requires representing math.MaxUint32 as an int, so the root package does not compile for 32-bit targets. Checking the signed lower bound first and comparing through uint64 avoids that compile-time overflow without changing the helper's contract.

Verification

  • baseline GOOS=linux GOARCH=386 go test -c . reproduces the overflow
  • candidate GOOS=linux GOARCH=386 go test -c . passes
  • CGO_ENABLED=0 GOOS=linux GOARCH=386 go build ./...
  • go test -count=1 ./...
  • go test -race -count=1 ./...
  • go build ./...
  • go vet ./...
  • go test -coverprofile=coverage.out -covermode=atomic ./...: repository 93.6%; changed coverable statements 100%
  • gofmt and git diff --check

Current CI status

The upstream fork workflow for head d0acbc8 is awaiting maintainer approval in Actions run 31433844937. Until that approval, GitHub cannot publish the repository's Actions or Codecov checks/comments. Local changed-line coverage is 100%.

@besmpl
besmpl marked this pull request as ready for review August 10, 2026 17:04
@besmpl
besmpl requested a review from kolkov as a code owner August 10, 2026 17:04

@kolkov kolkov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Confirmed — min(max(v, 0), math.MaxUint32) fails to compile on 32-bit targets because math.MaxUint32 overflows int. The uint64 comparison fix is correct and preserves the clamping contract on both 32-bit and 64-bit.

Tests adapt to the host int width, which is the right approach for a function that must work on both architectures.

LGTM, merging.

@codecov

codecov Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@kolkov

kolkov commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

CI fails on all three PRs with the same config issue:

jsonschema: "output" does not validate with "/properties/output/additionalProperties":
additional properties 'sort-results' not allowed

sort-results was deprecated in golangci-lint v1.63.0 and removed in v2 — results are now always sorted. The existing sort-order field in the config already controls the ordering.

Could you drop the sort-results: true addition from .golangci.yml in all three PRs?

@kolkov
kolkov merged commit f07e121 into gogpu:main Aug 12, 2026
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants