Skip to content

review: whole-repo comment budget, modern-Go idioms and named func types - #197

Merged
CMGS merged 3 commits into
masterfrom
review/whole-repo-2026-08-17
Aug 17, 2026
Merged

review: whole-repo comment budget, modern-Go idioms and named func types#197
CMGS merged 3 commits into
masterfrom
review/whole-repo-2026-08-17

Conversation

@CMGS

@CMGS CMGS commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Rebased onto master after #196 (net_scope) landed; the third commit refreshes docs/index.md (daemon-optional wording, mergeable memory, net_scope) so the docs match the tree.

What

Whole-repo /code + /simplify round with the comment budget as the primary lens. Every .go file (incl. _test.go) was walked; three mechanical scanners (comment audit ×2, rule scan) plus the four /simplify lenses (reuse, simplification, efficiency, altitude) fed candidates that were verified by hand before applying.

Three commits:

  1. comment budget pass (53 files, comment-only): every remaining multi-line // block joined to one line (showblocks = 30 → 0 repo-wide), restating/label/narration comments and interface-method godoc dropped, over-long package docs tightened. Prod comment lines 1426 → 1375; test comment lines trimmed accordingly.
  2. modern-Go idioms, named func types and shared-helper reuse (48 files):
    • cmp.Or / utils.OrDefault / min for hand-rolled fallbacks; slices.ContainsFunc/DeleteFunc/Backward; errors.AsType; errors.Is(fs.ErrNotExist); utils.FileExists over bare os.Stat probes.
    • Named func types for signatures spelled out repeatedly: hypervisor.VMOp (5 sites), images.PinRecheck (6 sites), localfile.BlobPinner.
    • Altitude fixes from the review lens: Backend.EffectiveCPUs (placement-vs-fence resolved once through the CgroupCPUFence seam instead of five backend sites reading the raw config field); --mergeable memory exclusion moved into types.VMConfig.Validate beside its sibling invariants; config.BridgeTAPPrefix()/NetnsPrefix() accessors so cmd/ never spells the scope→prefix rule; cgroup placement error names the persisted field.

Net: 92 files, +193 / −417.

Not changed (reviewed, judged not worth churn)

  • storageConfigToDisk's per-disk queueAffinity recompute (launch path, microseconds; a diskPolicy struct would touch six signatures).
  • Moving cgroup knob validation from the CLI constructors into the hypervisor entry points — the CLI fast-fail is deliberate and pinned by tests.
  • vm debug re-implementing half of buildCLIArgs — user-visible output format; separate change if wanted.
  • Two-sentence-on-one-line WHY comments flagged by the audit as "SHORTEN": already within the one-line budget.

Evidence

  • make lint GOOS=linux + darwin: 0 issues; asl ./... both GOOS: clean (pre-commit hook enforced on both commits)
  • go test ./... darwin: green
  • go test -count=1 ./... on Linux (docker golang:1.26.5-bookworm, linux/arm64): green — the amd64-emulated image fails daemon watcher (pidfd/epoll) and hypervisor stub-VMM exec tests identically on the base commit, i.e. an emulation artifact, not this branch

Base automatically changed from net/scoped-device-gc to master August 17, 2026 08:57
CMGS added 3 commits August 17, 2026 16:58
Join every remaining multi-line // block into the one-line budget, drop
comments that restate the code they sit on (labels, narration, name
echoes, interface-method godoc), and tighten package docs that had grown
past a sentence. Comment-only changes; no code moved.
Whole-repo /code + /simplify pass on top of the net_scope branch:

- cmp.Or / utils.OrDefault / min for hand-rolled fallbacks; slices
  ContainsFunc/DeleteFunc/Backward and errors.AsType for hand-rolled loops
  and errors.As+var; errors.Is(fs.ErrNotExist) over os.IsNotExist;
  utils.FileExists over bare os.Stat probes.
- Named func types where one signature was spelled out repeatedly:
  hypervisor.VMOp, images.PinRecheck, localfile.BlobPinner.
- Backend.EffectiveCPUs resolves placement-vs-fence through the
  CgroupCPUFence seam instead of five backend sites reaching into the raw
  config field; the --mergeable memory exclusion moves next to its sibling
  invariants in VMConfig.Validate; config gains BridgeTAPPrefix/NetnsPrefix
  accessors so cmd/ never spells the scope-to-prefix rule; cgroup's
  placement error names the persisted field, not a flag.
- Trailing/leading comment trims that rode along with the touched lines.
@CMGS
CMGS force-pushed the review/whole-repo-2026-08-17 branch from 61d9f32 to 4928404 Compare August 17, 2026 08:59
@CMGS
CMGS merged commit cde2318 into master Aug 17, 2026
4 checks passed
@CMGS
CMGS deleted the review/whole-repo-2026-08-17 branch August 17, 2026 09:04
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.

1 participant