Conversation
WalkthroughMove cloning/equality/zero-sized-type (ZST) utilities into a new internal/value package; remove local helpers; update nodes, tables, generated code, tests, and examples to use Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as API caller
participant Table as Table/Node
participant Value as internal/value
Note over Caller,Table: CloneRec(cloneFn value.CloneFunc[V])
Caller->>Table: CloneRec(cloneFn)
alt cloneFn == nil
Table->>Value: value.CloneFnFactory[V]()
Value-->>Table: cloneFn (maybe nil)
alt cloneFn == nil
Table->>Value: use value.CopyVal
Value-->>Table: copy function
end
else
Table-->>Table: use provided cloneFn
end
Table->>Table: apply cloneFn to payloads and recurse children
Table-->>Caller: cloned structure
sequenceDiagram
autonumber
participant Node as internal/nodes
participant Value as internal/value
Note over Node,Value: Decide payload printing in dump
Node->>Value: value.IsZST[V]()
Value-->>Node: isZST (bool)
alt isZST == false
Node->>Node: printValues = true
Node->>Node: include payloads in dump/FprintRec
else
Node->>Node: printValues = false
Node->>Node: omit payload sections
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Pull Request Test Coverage Report for Build 18918130677Details
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
internal/value/value.go (1)
19-22: Make ZST check allocation‑free using unsafe.Sizeof; keep heap fallback.Current approach allocates twice per call. Use a constant-time, zero‑alloc fast path and retain the existing pointer heuristic as a safety net.
Apply:
@@ -package value +package value + +import "unsafe" @@ -func IsZST[V any]() bool { - a, b := escapeToHeap[V]() - return a == b -} +func IsZST[V any]() bool { + var zero V + // Compile-time per-instantiation; no allocations. + if unsafe.Sizeof(zero) == 0 { + return true + } + // Fallback heuristic (defensive against future compiler changes). + a, b := escapeToHeap[V]() + return a == b +}Also applies to: 24-34
internal/value/value_test.go (1)
10-39: Optional: use subtests for clearer failures.Wrap cases with t.Run to pinpoint which type failed and allow per‑case parallelism.
func TestIsZeroSizedType(t *testing.T) { - t.Parallel() + t.Parallel() @@ - for _, tt := range tests { - if tt.got != tt.want { - t.Errorf("%s, want %v, got %v", tt.name, tt.want, tt.got) - } - } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if tt.got != tt.want { + t.Fatalf("want %v, got %v", tt.want, tt.got) + } + }) + }internal/nodes/litemethodsgenerated.go (1)
1075-1103: Avoid calling IsZST inside the loop; compute once.IsZST currently allocates; doing it per item is wasteful. Compute once before iterating.
@@ func (n *LiteNode[V]) FprintRec(w io.Writer, parent TrieItem[V], pad string) error { - // for all direct item under this node ... + // for all direct item under this node ... + printVal := !value.IsZST[V]() // compute once for i, item := range directItems { @@ - var err error - // val is the empty struct, don't print it - switch { - case value.IsZST[V](): // skip printing values if V is zero-sized - _, err = fmt.Fprintf(w, "%s%s\n", pad+glyph, item.Cidr) - default: - _, err = fmt.Fprintf(w, "%s%s (%v)\n", pad+glyph, item.Cidr, item.Val) - } + var err error + if printVal { + _, err = fmt.Fprintf(w, "%s%s (%v)\n", pad+glyph, item.Cidr, item.Val) + } else { + _, err = fmt.Fprintf(w, "%s%s\n", pad+glyph, item.Cidr) + }internal/nodes/bartmethodsgenerated.go (1)
1083-1103: Precompute printVal in FprintRec to avoid repeated IsZST calls.Same optimization as Lite: compute once before the loop.
@@ func (n *BartNode[V]) FprintRec(w io.Writer, parent TrieItem[V], pad string) error { - // for all direct item under this node ... + // for all direct item under this node ... + printVal := !value.IsZST[V]() for i, item := range directItems { @@ - var err error - // val is the empty struct, don't print it - switch { - case value.IsZST[V](): // skip printing values if V is zero-sized - _, err = fmt.Fprintf(w, "%s%s\n", pad+glyph, item.Cidr) - default: - _, err = fmt.Fprintf(w, "%s%s (%v)\n", pad+glyph, item.Cidr, item.Val) - } + var err error + if printVal { + _, err = fmt.Fprintf(w, "%s%s (%v)\n", pad+glyph, item.Cidr, item.Val) + } else { + _, err = fmt.Fprintf(w, "%s%s\n", pad+glyph, item.Cidr) + }internal/nodes/commonmethods_tmpl.go (1)
1117-1146: Fix template to compute printVal once; generated code will follow suit.Precomputing avoids per‑item allocations in all generated variants.
@@ func (n *_NODE_TYPE[V]) FprintRec(w io.Writer, parent TrieItem[V], pad string) error { - // for all direct item under this node ... + // for all direct item under this node ... + printVal := !value.IsZST[V]() for i, item := range directItems { @@ - var err error - // val is the empty struct, don't print it - switch { - case value.IsZST[V](): // skip printing values if V is zero-sized - _, err = fmt.Fprintf(w, "%s%s\n", pad+glyph, item.Cidr) - default: - _, err = fmt.Fprintf(w, "%s%s (%v)\n", pad+glyph, item.Cidr, item.Val) - } + var err error + if printVal { + _, err = fmt.Fprintf(w, "%s%s (%v)\n", pad+glyph, item.Cidr, item.Val) + } else { + _, err = fmt.Fprintf(w, "%s%s\n", pad+glyph, item.Cidr) + }internal/nodes/fastmethodsgenerated.go (1)
1083-1103: Precompute printVal in FprintRec; avoid repeated allocations.Mirror the Lite/Bart change here.
@@ func (n *FastNode[V]) FprintRec(w io.Writer, parent TrieItem[V], pad string) error { - // for all direct item under this node ... + // for all direct item under this node ... + printVal := !value.IsZST[V]() for i, item := range directItems { @@ - var err error - // val is the empty struct, don't print it - switch { - case value.IsZST[V](): // skip printing values if V is zero-sized - _, err = fmt.Fprintf(w, "%s%s\n", pad+glyph, item.Cidr) - default: - _, err = fmt.Fprintf(w, "%s%s (%v)\n", pad+glyph, item.Cidr, item.Val) - } + var err error + if printVal { + _, err = fmt.Fprintf(w, "%s%s (%v)\n", pad+glyph, item.Cidr, item.Val) + } else { + _, err = fmt.Fprintf(w, "%s%s\n", pad+glyph, item.Cidr) + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
common.go(2 hunks)internal/nodes/bartmethodsgenerated.go(3 hunks)internal/nodes/commonmethods_tmpl.go(3 hunks)internal/nodes/fastmethodsgenerated.go(3 hunks)internal/nodes/litemethodsgenerated.go(3 hunks)internal/nodes/nodebasics.go(0 hunks)internal/value/value.go(1 hunks)internal/value/value_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/nodes/nodebasics.go
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-10-05T21:46:51.386Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:46:51.386Z
Learning: In internal/nodes, FastNode and LiteNode expose DumpString(octets []uint8, depth int, is4 bool, printVals bool) string with identical traversal/error semantics as BartNode; tests must construct multi-level paths via InsertChild with node-typed children to avoid Fringe/Leaf along the traversal path, and assert the two error branches (kid not set, kid wrong type).
Applied to files:
internal/nodes/commonmethods_tmpl.gointernal/nodes/fastmethodsgenerated.gointernal/nodes/bartmethodsgenerated.gointernal/nodes/litemethodsgenerated.go
📚 Learning: 2025-10-05T21:26:24.231Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:26:24.231Z
Learning: BartNode DumpString signature in internal/nodes: DumpString(octets []uint8, depth int, is4 bool, printVals bool) string. It traverses down the child path (expecting *BartNode at each step) before dumping the subtree with Dump. Tests must pass octets (first N octets) and a matching depth to target deeper nodes.
Applied to files:
internal/nodes/commonmethods_tmpl.gointernal/nodes/fastmethodsgenerated.gointernal/nodes/bartmethodsgenerated.gointernal/nodes/litemethodsgenerated.go
📚 Learning: 2025-10-05T21:27:32.948Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:27:32.948Z
Learning: BartNode DumpString signature in internal/nodes: func (n *BartNode[V]) DumpString(octets []uint8, depth int, is4 bool, printVals bool) string. It traverses down the child chain using GetChild(path[i]) and requires BartNode children along the octet/depth path; otherwise it returns an "ERROR: kid ..." string. Tests must ensure traversal paths are BartNode nodes (e.g., by inserting BartNode children via InsertChild) instead of Fringe/Leaf children.
Applied to files:
internal/nodes/commonmethods_tmpl.gointernal/nodes/fastmethodsgenerated.gointernal/nodes/bartmethodsgenerated.gointernal/nodes/litemethodsgenerated.go
📚 Learning: 2025-09-17T13:02:04.787Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-17T13:02:04.787Z
Learning: In the gaissmai/bart codebase, all types including public types Table, Fat, and Lite are designed to work with zero values. Use &Table[T]{}, &Fat[T]{}, &Lite[T]{} instead of constructor functions NewTable[T](), NewFat[T](), NewLite[T]() in tests and code.
Applied to files:
internal/value/value_test.go
📚 Learning: 2025-10-17T18:36:43.515Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-17T18:36:43.515Z
Learning: In the bart package (all.go), the blank import of `internal/bitset` at the top level is essential for proper inlining of BitSet256 functions in production code. Without it, the Go inliner fails due to the deep nesting path (bart -> internal/nodes -> internal/sparse -> internal/bitset). This has been verified via CPU profiling.
Applied to files:
common.go
📚 Learning: 2025-09-13T19:12:00.916Z
Learnt from: gaissmai
PR: gaissmai/bart#291
File: internal/sparse/array256.go:53-56
Timestamp: 2025-09-13T19:12:00.916Z
Learning: In the gaissmai/bart project, Array256[T] intentionally embeds bitset.BitSet256 as a public field to enable direct, high-performance bitset operations in critical code paths. Examples include direct bitset comparisons in equal.go (lines 37, 41) and direct bitset intersection/union operations in overlaps.go (lines 84, 98, 189, 192, 203, 366, 373). The embedded field design allows zero-cost abstraction for bitset operations and should never be refactored to a private field, as this would significantly hurt performance in hot paths.
Applied to files:
common.go
📚 Learning: 2025-09-13T19:12:00.916Z
Learnt from: gaissmai
PR: gaissmai/bart#291
File: internal/sparse/array256.go:53-56
Timestamp: 2025-09-13T19:12:00.916Z
Learning: In the gaissmai/bart project, the Array256[T] struct intentionally embeds bitset.BitSet256 as a public field to allow direct, efficient access to bitset operations in performance-critical code paths (e.g., in equal.go and overlaps.go). This design prioritizes performance over the safety concern of potential invariant-breaking mutations, and should not be suggested for refactoring.
Applied to files:
common.go
🧬 Code graph analysis (6)
internal/nodes/commonmethods_tmpl.go (1)
internal/value/value.go (1)
IsZST(19-22)
internal/value/value_test.go (1)
internal/value/value.go (1)
IsZST(19-22)
internal/nodes/fastmethodsgenerated.go (1)
internal/value/value.go (1)
IsZST(19-22)
common.go (1)
internal/value/value.go (1)
IsZST(19-22)
internal/nodes/bartmethodsgenerated.go (1)
internal/value/value.go (1)
IsZST(19-22)
internal/nodes/litemethodsgenerated.go (1)
internal/value/value.go (1)
IsZST(19-22)
🔇 Additional comments (1)
common.go (1)
31-33: Centralizing ZST check in value package looks good.Import and panicOnZST change are correct and keep behavior intact.
Also applies to: 94-97
@coderabbitai unsafe ist hier keine option, die zwei heap allocations passieren nicht im hot path |
|
Tip For best results, initiate chat on the files or code changes. @gaissmai: Understood! If the ZST check isn't in the hot path and Thank you for the clarification! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/nodes/commonmethods_tmpl.go (1)
841-841: Dump: conditional value printing via value.IsZSTGood gating; keeps debug output clean for zero‑sized V. Optional: rename printVal → printValues for clarity.
internal/nodes/bartmethodsgenerated.go (1)
807-807: Dump: gate value printing with value.IsZSTSolid. Optional: unify formatting of values across prefixes/leaves/fringes (use either %v or %#v consistently).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
common.go(2 hunks)internal/nodes/bartmethodsgenerated.go(4 hunks)internal/nodes/commonmethods_tmpl.go(4 hunks)internal/nodes/fastmethodsgenerated.go(4 hunks)internal/nodes/litemethodsgenerated.go(4 hunks)internal/nodes/nodebasics.go(0 hunks)internal/value/value.go(1 hunks)internal/value/value_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/nodes/nodebasics.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/nodes/litemethodsgenerated.go
- internal/value/value.go
- internal/value/value_test.go
- common.go
- internal/nodes/fastmethodsgenerated.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-05T21:26:24.231Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:26:24.231Z
Learning: BartNode DumpString signature in internal/nodes: DumpString(octets []uint8, depth int, is4 bool, printVals bool) string. It traverses down the child path (expecting *BartNode at each step) before dumping the subtree with Dump. Tests must pass octets (first N octets) and a matching depth to target deeper nodes.
Applied to files:
internal/nodes/bartmethodsgenerated.gointernal/nodes/commonmethods_tmpl.go
📚 Learning: 2025-10-05T21:46:51.386Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:46:51.386Z
Learning: In internal/nodes, FastNode and LiteNode expose DumpString(octets []uint8, depth int, is4 bool, printVals bool) string with identical traversal/error semantics as BartNode; tests must construct multi-level paths via InsertChild with node-typed children to avoid Fringe/Leaf along the traversal path, and assert the two error branches (kid not set, kid wrong type).
Applied to files:
internal/nodes/bartmethodsgenerated.gointernal/nodes/commonmethods_tmpl.go
📚 Learning: 2025-10-05T21:27:32.948Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:27:32.948Z
Learning: BartNode DumpString signature in internal/nodes: func (n *BartNode[V]) DumpString(octets []uint8, depth int, is4 bool, printVals bool) string. It traverses down the child chain using GetChild(path[i]) and requires BartNode children along the octet/depth path; otherwise it returns an "ERROR: kid ..." string. Tests must ensure traversal paths are BartNode nodes (e.g., by inserting BartNode children via InsertChild) instead of Fringe/Leaf children.
Applied to files:
internal/nodes/bartmethodsgenerated.gointernal/nodes/commonmethods_tmpl.go
🧬 Code graph analysis (2)
internal/nodes/bartmethodsgenerated.go (1)
internal/value/value.go (1)
IsZST(19-22)
internal/nodes/commonmethods_tmpl.go (1)
internal/value/value.go (1)
IsZST(19-22)
🔇 Additional comments (3)
internal/nodes/commonmethods_tmpl.go (2)
26-26: Centralized ZST check import looks goodSwitching to internal/value aligns ZST logic in one place. No behavior change for non‑ZST.
1117-1119: FprintRec: skip values for ZST typesLGTM. Please ensure tests exercise both branches (ZST and non‑ZST) to avoid coverage gaps in generated files.
Also applies to: 1134-1139
internal/nodes/bartmethodsgenerated.go (1)
17-17: Import internal/value for ZST detection — OKConsistent with refactor; keeps generated code dependency‑free from unsafe.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
internal/nodes/barttestsgenerated_test.go (1)
663-671: Optional: run the ZST FprintRec test in parallel.Match the rest of the suite for consistency.
func TestFprintRec_ZST_BartNode(t *testing.T) { - var node BartNode[struct{}] + t.Parallel() + var node BartNode[struct{}]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
internal/nodes/bartmethodsgenerated.go(9 hunks)internal/nodes/barttestsgenerated_test.go(8 hunks)internal/nodes/commonmethods_tmpl.go(9 hunks)internal/nodes/commontests_tmpl.go(8 hunks)internal/nodes/fastmethodsgenerated.go(9 hunks)internal/nodes/fasttestsgenerated_test.go(8 hunks)internal/nodes/litemethodsgenerated.go(9 hunks)internal/nodes/litetestsgenerated_test.go(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/nodes/commonmethods_tmpl.go
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-10-05T21:46:51.386Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:46:51.386Z
Learning: In internal/nodes, FastNode and LiteNode expose DumpString(octets []uint8, depth int, is4 bool, printVals bool) string with identical traversal/error semantics as BartNode; tests must construct multi-level paths via InsertChild with node-typed children to avoid Fringe/Leaf along the traversal path, and assert the two error branches (kid not set, kid wrong type).
Applied to files:
internal/nodes/commontests_tmpl.gointernal/nodes/bartmethodsgenerated.gointernal/nodes/litemethodsgenerated.gointernal/nodes/fasttestsgenerated_test.gointernal/nodes/fastmethodsgenerated.gointernal/nodes/barttestsgenerated_test.gointernal/nodes/litetestsgenerated_test.go
📚 Learning: 2025-10-05T21:26:24.231Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:26:24.231Z
Learning: BartNode DumpString signature in internal/nodes: DumpString(octets []uint8, depth int, is4 bool, printVals bool) string. It traverses down the child path (expecting *BartNode at each step) before dumping the subtree with Dump. Tests must pass octets (first N octets) and a matching depth to target deeper nodes.
Applied to files:
internal/nodes/commontests_tmpl.gointernal/nodes/bartmethodsgenerated.gointernal/nodes/litemethodsgenerated.gointernal/nodes/fasttestsgenerated_test.gointernal/nodes/fastmethodsgenerated.gointernal/nodes/barttestsgenerated_test.gointernal/nodes/litetestsgenerated_test.go
📚 Learning: 2025-10-05T21:27:32.948Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:27:32.948Z
Learning: BartNode DumpString signature in internal/nodes: func (n *BartNode[V]) DumpString(octets []uint8, depth int, is4 bool, printVals bool) string. It traverses down the child chain using GetChild(path[i]) and requires BartNode children along the octet/depth path; otherwise it returns an "ERROR: kid ..." string. Tests must ensure traversal paths are BartNode nodes (e.g., by inserting BartNode children via InsertChild) instead of Fringe/Leaf children.
Applied to files:
internal/nodes/bartmethodsgenerated.gointernal/nodes/litemethodsgenerated.gointernal/nodes/fastmethodsgenerated.gointernal/nodes/barttestsgenerated_test.go
📚 Learning: 2025-10-05T21:31:48.381Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:31:48.381Z
Learning: In internal/nodes BartNode, inserting prefixes like /8, /16, /24 via Insert creates Fringe/Leaf, never normal BartNode nodes. For DumpString tests that must traverse deeper via octets/depth, construct the path with InsertChild to attach BartNode children; populate those nodes via InsertPrefix so the dump has content.
Applied to files:
internal/nodes/bartmethodsgenerated.go
📚 Learning: 2025-09-12T06:23:05.913Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-12T06:23:05.913Z
Learning: The leafNode[V] struct in this codebase has two fields: `prefix netip.Prefix` and `value V`. The prefix field is of type netip.Prefix, not []byte. When generating test code for leafNode, use netip.MustParsePrefix() or similar functions to create valid netip.Prefix values.
Applied to files:
internal/nodes/bartmethodsgenerated.gointernal/nodes/litemethodsgenerated.gointernal/nodes/fastmethodsgenerated.go
📚 Learning: 2025-10-17T18:36:43.515Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-17T18:36:43.515Z
Learning: In the bart package (all.go), the blank import of `internal/bitset` at the top level is essential for proper inlining of BitSet256 functions in production code. Without it, the Go inliner fails due to the deep nesting path (bart -> internal/nodes -> internal/sparse -> internal/bitset). This has been verified via CPU profiling.
Applied to files:
internal/nodes/barttestsgenerated_test.go
📚 Learning: 2025-09-19T07:28:23.239Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-19T07:28:23.239Z
Learning: In the bart repository nodeiface_test.go file, the constant declarations like `const c = uint8(100)` at line 984 are already properly scoped within their respective test functions and should not be moved.
Applied to files:
internal/nodes/barttestsgenerated_test.go
📚 Learning: 2025-09-30T08:30:12.716Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-30T08:30:12.716Z
Learning: In the bart tests, prefer the existing helpers mpp (MustParsePrefix) and mpa (MustParseAddr); do not re-declare local equivalents in new *_test.go files.
Applied to files:
internal/nodes/barttestsgenerated_test.go
📚 Learning: 2025-10-05T07:11:38.369Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T07:11:38.369Z
Learning: LiteNode in the bart codebase does not use the allot mechanism like FastNode does. LiteNode.Contains() checks for ancestor prefixes in the complete binary tree using LookupTbl, not descendants. When testing LiteNode, ensure tests reflect ancestor relationships (idx, idx>>1, idx>>2, ...) rather than descendant propagation (idx<<1, (idx<<1)+1, ...).
Applied to files:
internal/nodes/litetestsgenerated_test.go
📚 Learning: 2025-10-05T19:56:18.121Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T19:56:18.121Z
Learning: In internal/nodes LiteNode does not store values; tests for Modify/Insert/Get must assert existence (ok) rather than value equality. Prefer Get(pfx) for verification and avoid asserting returned values.
Applied to files:
internal/nodes/litetestsgenerated_test.go
🧬 Code graph analysis (7)
internal/nodes/commontests_tmpl.go (2)
internal/nodes/nodebasics.go (2)
StridePath(33-33)TrieItem(37-48)internal/nodes/lite.go (1)
LiteNode(62-69)
internal/nodes/bartmethodsgenerated.go (3)
internal/nodes/bart.go (1)
BartNode(26-43)internal/nodes/nodebasics.go (1)
StridePath(33-33)internal/value/value.go (1)
IsZST(19-22)
internal/nodes/litemethodsgenerated.go (2)
internal/nodes/lite.go (1)
LiteNode(62-69)internal/value/value.go (1)
IsZST(19-22)
internal/nodes/fasttestsgenerated_test.go (3)
internal/nodes/fast.go (1)
FastNode(21-51)internal/nodes/nodebasics.go (2)
StridePath(33-33)TrieItem(37-48)internal/nodes/lite.go (1)
LiteNode(62-69)
internal/nodes/fastmethodsgenerated.go (3)
internal/nodes/fast.go (1)
FastNode(21-51)internal/nodes/nodebasics.go (1)
StridePath(33-33)internal/value/value.go (1)
IsZST(19-22)
internal/nodes/barttestsgenerated_test.go (3)
internal/nodes/bart.go (1)
BartNode(26-43)internal/nodes/nodebasics.go (2)
StridePath(33-33)TrieItem(37-48)internal/nodes/lite.go (1)
LiteNode(62-69)
internal/nodes/litetestsgenerated_test.go (2)
internal/nodes/lite.go (1)
LiteNode(62-69)internal/nodes/nodebasics.go (2)
StridePath(33-33)TrieItem(37-48)
🔇 Additional comments (6)
internal/nodes/commontests_tmpl.go (1)
16-16: LGTM on import addition and test function renames.The
bytesimport supports the newbytes.Bufferusage in FprintRec tests, and the test naming convention changes (adding double underscores) appear intentional and consistent with the code generation pattern.Also applies to: 84-84, 371-371, 421-421, 454-454, 487-487, 561-561
internal/nodes/fasttestsgenerated_test.go (1)
9-9: LGTM on import addition and test function renames.Generated code correctly applies the template changes for FastNode.
Also applies to: 58-58, 345-345, 395-395, 428-428, 461-461, 535-535
internal/nodes/litetestsgenerated_test.go (1)
9-9: LGTM on import addition and test function renames.Generated code correctly applies the template changes for LiteNode.
Also applies to: 58-58, 345-345, 395-395, 428-428, 461-461, 535-535
internal/nodes/litemethodsgenerated.go (1)
17-18: Verification confirms refactor is complete; no stale Dump() call sites remain.The search for exported
Dump()method calls produced no results, confirming the refactor successfully replaced all external Dump calls with the new pattern. Exported methodsDumpRecandDumpStringnow properly delegate to unexporteddump()helpers, with ZST-aware printing controlled by!value.IsZST[V]()checks. The internal calls todump()are correct and test calls are appropriate.internal/nodes/fastmethodsgenerated.go (1)
17-18: Code changes verify correctly—consistent implementation across FastNode, LiteNode, and BartNode.DumpString properly delegates to dump without exposing printVals as a parameter (correct design), and dump internally gates value printing via
value.IsZST[V](). All three node types follow identical patterns. Imports are correct and necessary.internal/nodes/bartmethodsgenerated.go (1)
17-18: Review comment is accurate and should stand as written.All claims verified:
printValues := !value.IsZST[V]()correctly gates value printing at line 807 (in privatedumpmethod)DumpRec(line 781) andDumpString(line 938) both unify through the privatedumpmethod, ensuring consistent behavior- No public
Dump(...)invocations remain on BartNode- Refactoring from learnings notes:
printValsparameter removed from public signatures; computation moved to privatedump()whereprintValuesis computed once and reused at lines 831, 882, 902
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
internal/nodes/fasttestsgenerated_test.go (1)
637-641: Skip logic for LiteNode is now correct.Condition uses isLite (no negation), so FastNode tests won’t skip and LiteNode-specific guards remain harmless here. LGTM.
Also applies to: 697-701
internal/nodes/commontests_tmpl.go (2)
657-666: LiteNode skip condition fixed and generalized across templates.This prevents unintended skips/runs when instantiating for different node types. LGTM.
Also applies to: 717-726
711-714: ZST parenthesis check is precise.Template now checks prefix+paren together; avoids false positives.
🧹 Nitpick comments (5)
internal/nodes/litetestsgenerated_test.go (2)
631-641: These non‑ZST LiteNode tests always skip.Optional: drop or guard them at generation time for Lite to reduce noise; current skips are fine.
Based on learnings
Also applies to: 691-701
663-669: Consistency nit: add t.Parallel().Other tests in this file run in parallel; consider adding t.Parallel() here too.
internal/nodes/litemethodsgenerated.go (1)
926-938: Doc: DumpString says “via Dump” but calls dump.Align the comment to avoid confusion.
-// DumpString traverses the trie to the node at the specified depth along the given -// octet path and returns its string representation via Dump. +// DumpString traverses the trie to the node at the specified depth along the given +// octet path and returns its string representation via dump.internal/nodes/commontests_tmpl.go (1)
31-44: Add a dump() stub for _NODE_TYPE to satisfy gopls in generate-only file.Keeps editor happy without affecting builds.
func (n *_NODE_TYPE[V]) AllRecSorted(StridePath, int, bool, func(netip.Prefix, V) bool) (_ bool) { return } +// dump is used by generated tests; stubbed for gopls under the generate build tag. +func (n *_NODE_TYPE[V]) dump(io.Writer, StridePath, int, bool) { return }Also applies to: 645-648, 671-674
internal/nodes/bartmethodsgenerated.go (1)
926-938: Doc: DumpString says “via Dump” but calls dump.Adjust wording for accuracy.
-// DumpString traverses the trie to the node at the specified depth along the given -// octet path and returns its string representation via Dump. +// DumpString traverses the trie to the node at the specified depth along the given +// octet path and returns its string representation via dump.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
internal/nodes/bartmethodsgenerated.go(9 hunks)internal/nodes/barttestsgenerated_test.go(8 hunks)internal/nodes/commonmethods_tmpl.go(9 hunks)internal/nodes/commontests_tmpl.go(8 hunks)internal/nodes/fastmethodsgenerated.go(9 hunks)internal/nodes/fasttestsgenerated_test.go(8 hunks)internal/nodes/litemethodsgenerated.go(9 hunks)internal/nodes/litetestsgenerated_test.go(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/nodes/fastmethodsgenerated.go
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-10-05T21:46:51.386Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:46:51.386Z
Learning: In internal/nodes, FastNode and LiteNode expose DumpString(octets []uint8, depth int, is4 bool, printVals bool) string with identical traversal/error semantics as BartNode; tests must construct multi-level paths via InsertChild with node-typed children to avoid Fringe/Leaf along the traversal path, and assert the two error branches (kid not set, kid wrong type).
Applied to files:
internal/nodes/litemethodsgenerated.gointernal/nodes/commontests_tmpl.gointernal/nodes/fasttestsgenerated_test.gointernal/nodes/litetestsgenerated_test.gointernal/nodes/barttestsgenerated_test.gointernal/nodes/bartmethodsgenerated.gointernal/nodes/commonmethods_tmpl.go
📚 Learning: 2025-10-05T21:27:32.948Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:27:32.948Z
Learning: BartNode DumpString signature in internal/nodes: func (n *BartNode[V]) DumpString(octets []uint8, depth int, is4 bool, printVals bool) string. It traverses down the child chain using GetChild(path[i]) and requires BartNode children along the octet/depth path; otherwise it returns an "ERROR: kid ..." string. Tests must ensure traversal paths are BartNode nodes (e.g., by inserting BartNode children via InsertChild) instead of Fringe/Leaf children.
Applied to files:
internal/nodes/litemethodsgenerated.gointernal/nodes/barttestsgenerated_test.gointernal/nodes/bartmethodsgenerated.gointernal/nodes/commonmethods_tmpl.go
📚 Learning: 2025-10-05T21:26:24.231Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:26:24.231Z
Learning: BartNode DumpString signature in internal/nodes: DumpString(octets []uint8, depth int, is4 bool, printVals bool) string. It traverses down the child path (expecting *BartNode at each step) before dumping the subtree with Dump. Tests must pass octets (first N octets) and a matching depth to target deeper nodes.
Applied to files:
internal/nodes/litemethodsgenerated.gointernal/nodes/commontests_tmpl.gointernal/nodes/fasttestsgenerated_test.gointernal/nodes/litetestsgenerated_test.gointernal/nodes/barttestsgenerated_test.gointernal/nodes/bartmethodsgenerated.gointernal/nodes/commonmethods_tmpl.go
📚 Learning: 2025-09-12T06:23:05.913Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-12T06:23:05.913Z
Learning: The leafNode[V] struct in this codebase has two fields: `prefix netip.Prefix` and `value V`. The prefix field is of type netip.Prefix, not []byte. When generating test code for leafNode, use netip.MustParsePrefix() or similar functions to create valid netip.Prefix values.
Applied to files:
internal/nodes/litemethodsgenerated.gointernal/nodes/bartmethodsgenerated.gointernal/nodes/commonmethods_tmpl.go
📚 Learning: 2025-10-05T07:11:38.369Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T07:11:38.369Z
Learning: LiteNode in the bart codebase does not use the allot mechanism like FastNode does. LiteNode.Contains() checks for ancestor prefixes in the complete binary tree using LookupTbl, not descendants. When testing LiteNode, ensure tests reflect ancestor relationships (idx, idx>>1, idx>>2, ...) rather than descendant propagation (idx<<1, (idx<<1)+1, ...).
Applied to files:
internal/nodes/commontests_tmpl.gointernal/nodes/fasttestsgenerated_test.gointernal/nodes/litetestsgenerated_test.gointernal/nodes/barttestsgenerated_test.go
📚 Learning: 2025-10-05T07:12:24.637Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T07:12:24.637Z
Learning: LiteNode in the bart codebase does not use the allot mechanism like FastNode does. LiteNode.Contains() checks for ancestor prefixes in the complete binary tree using LookupTbl[idx], not descendants. When testing LiteNode, ensure tests reflect ancestor relationships (idx, idx>>1, idx>>2, ...) rather than descendant propagation. The allot table mentioned by the maintainer (32→64,65,128,...) applies to FastNode's descendant propagation, not LiteNode's ancestor checking.
Applied to files:
internal/nodes/commontests_tmpl.gointernal/nodes/fasttestsgenerated_test.gointernal/nodes/litetestsgenerated_test.gointernal/nodes/barttestsgenerated_test.go
📚 Learning: 2025-10-19T06:22:03.283Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-19T06:22:03.283Z
Learning: Tests that verify modified payload values should skip liteTable because liteTable uses struct{} as its payload type and has no real payload to verify modifications on.
Applied to files:
internal/nodes/commontests_tmpl.gointernal/nodes/litetestsgenerated_test.gointernal/nodes/barttestsgenerated_test.go
📚 Learning: 2025-10-05T19:56:18.121Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T19:56:18.121Z
Learning: In internal/nodes LiteNode does not store values; tests for Modify/Insert/Get must assert existence (ok) rather than value equality. Prefer Get(pfx) for verification and avoid asserting returned values.
Applied to files:
internal/nodes/commontests_tmpl.gointernal/nodes/litetestsgenerated_test.go
📚 Learning: 2025-09-30T08:30:12.716Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-30T08:30:12.716Z
Learning: In the bart tests, prefer the existing helpers mpp (MustParsePrefix) and mpa (MustParseAddr); do not re-declare local equivalents in new *_test.go files.
Applied to files:
internal/nodes/barttestsgenerated_test.go
📚 Learning: 2025-09-19T07:28:23.239Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-19T07:28:23.239Z
Learning: In the bart repository nodeiface_test.go file, the constant declarations like `const c = uint8(100)` at line 984 are already properly scoped within their respective test functions and should not be moved.
Applied to files:
internal/nodes/barttestsgenerated_test.go
📚 Learning: 2025-10-05T21:31:48.381Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:31:48.381Z
Learning: In internal/nodes BartNode, inserting prefixes like /8, /16, /24 via Insert creates Fringe/Leaf, never normal BartNode nodes. For DumpString tests that must traverse deeper via octets/depth, construct the path with InsertChild to attach BartNode children; populate those nodes via InsertPrefix so the dump has content.
Applied to files:
internal/nodes/barttestsgenerated_test.gointernal/nodes/bartmethodsgenerated.go
🧬 Code graph analysis (7)
internal/nodes/litemethodsgenerated.go (3)
internal/nodes/lite.go (1)
LiteNode(62-69)internal/nodes/nodebasics.go (1)
StridePath(33-33)internal/value/value.go (1)
IsZST(19-22)
internal/nodes/commontests_tmpl.go (2)
internal/nodes/nodebasics.go (2)
StridePath(33-33)TrieItem(37-48)internal/nodes/lite.go (1)
LiteNode(62-69)
internal/nodes/fasttestsgenerated_test.go (3)
internal/nodes/fast.go (1)
FastNode(21-51)internal/nodes/nodebasics.go (2)
StridePath(33-33)TrieItem(37-48)internal/nodes/lite.go (1)
LiteNode(62-69)
internal/nodes/litetestsgenerated_test.go (2)
internal/nodes/lite.go (1)
LiteNode(62-69)internal/nodes/nodebasics.go (2)
StridePath(33-33)TrieItem(37-48)
internal/nodes/barttestsgenerated_test.go (3)
internal/nodes/bart.go (1)
BartNode(26-43)internal/nodes/nodebasics.go (2)
StridePath(33-33)TrieItem(37-48)internal/nodes/lite.go (1)
LiteNode(62-69)
internal/nodes/bartmethodsgenerated.go (3)
internal/nodes/bart.go (1)
BartNode(26-43)internal/nodes/nodebasics.go (1)
StridePath(33-33)internal/value/value.go (1)
IsZST(19-22)
internal/nodes/commonmethods_tmpl.go (2)
internal/nodes/nodebasics.go (1)
StridePath(33-33)internal/value/value.go (1)
IsZST(19-22)
🔇 Additional comments (17)
internal/nodes/fasttestsgenerated_test.go (1)
685-689: ZST parenthesis check is precise.Looking for "10.0.0.0/7 (" avoids false positives. Good.
internal/nodes/litetestsgenerated_test.go (1)
685-689: ZST parenthesis check is precise.Combined prefix+paren match avoids misfires. LGTM.
internal/nodes/litemethodsgenerated.go (4)
786-788: Value printing gated by value.IsZST — correct and contained.printValues derived from value.IsZSTV is applied consistently to prefixes, leaves, and fringes. LGTM.
Also applies to: 802-843, 881-907
958-960: Unexported dump is correctly used by DumpString/DumpRec.Internalizing dump is sound; call sites updated.
1083-1105: FprintRec ZST gating looks good and nil‑receiver safe.Calling nextNode.FprintRec with a nil receiver is guarded by the early nil check. LGTM.
1-18: All concerns verified—code is consistent.The verification confirms:
- No public
Dump()call sites exist outside the packageIsZST[V]()is consistently used with proper package qualification (value.IsZST[V]())- Private
dump()method usage is confined to templates, generated files, and tests as expectedNo action required.
internal/nodes/bartmethodsgenerated.go (3)
786-788: Value printing gated by value.IsZST — applied consistently.Prefixes, leaves, and fringes respect printValues. LGTM.
Also applies to: 802-843, 881-907
958-960: DumpString delegates to dump — correct internalization.Matches the refactor to unexported dump.
1083-1105: FprintRec ZST gating and recursion behavior look good.Print conditional aligns with tests; nil receiver guard is present in method.
internal/nodes/commonmethods_tmpl.go (4)
26-26: LGTM! Import aligns with centralized ZST detection.The new import supports the refactored zero-sized type checks via
value.IsZST[V]()throughout this file.
821-821: LGTM! Dump internalization and ZST-aware printing are correct.The refactoring correctly:
- Renames
Dumpto unexporteddump- Uses centralized
value.IsZST[V]()to computeprintValues- Conditionally emits values only when V is not zero-sized
The implementation is consistent with the PR's goal of moving ZST detection to
internal/value.Also applies to: 832-841, 865-865, 916-920, 936-940
992-992: LGTM! DumpString call site correctly updated.The call to
n.dumpaligns with the method rename;DumpStringremains exported and delegates to the internal implementation.
1117-1139: LGTM! FprintRec correctly adopts ZST-aware printing.The
printValuesflag computed viavalue.IsZST[V]()ensures values are only printed for non-zero-sized types, consistent with thedumpmethod.internal/nodes/barttestsgenerated_test.go (4)
9-9: LGTM! Import and test naming updates are correct.The
bytesimport supports the new test functions, and the underscore convention in test names improves readability.Also applies to: 58-58, 345-345, 395-395, 428-428, 461-461, 535-535
610-629: LGTM! ZST dump test correctly validates omitted values.The test properly verifies that when
Vis a zero-sized type (struct{}), the dump output omits the "values()" section, as expected.
632-660: LGTM! Non-ZST dump test correctly validates printed values.The test properly verifies that when
Vis a non-zero-sized type (int), the dump output includes the "values()" section with the actual value (42). The LiteNode check (lines 638-640) is now correct (usingisLiteinstead of!isLiteas flagged in past reviews).
663-727: LGTM! FprintRec tests correctly validate ZST/non-ZST behavior.Both tests properly verify the conditional printing behavior:
- ZST test (663-689): Confirms no value is printed in parentheses for
struct{}- NonZST test (692-727): Confirms both prefix and value (
testval) are printed forstringThe LiteNode check (lines 698-700) is now correct (using
isLiteinstead of!isLiteas flagged in past reviews).
|
@coderabbitai ah Danke für den Stub, FprintRec hat auch noch gefehlt. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1335 @gaissmai: Gerne! Here's a package docstring for // Package value provides utilities for working with generic type parameters
// at runtime.
//
// The primary functionality is zero-sized type (ZST) detection, which allows
// code to optimize behavior based on whether a generic type parameter V
// occupies memory. Types like struct{} and [0]byte are ZSTs and can be
// handled specially to avoid unnecessary storage or marshaling overhead.
//
// This is an internal package used by the bart data structure implementation.
package valueThis docstring:
Feel free to adjust the wording to match your preferred style! |
@coderabbitai hört sich solide an, was mir aber fehlt, ist dass ich dass nicht nur fürs dumpen und printen benötige sondern auch und vor allem für einen runtime check, damit man Fast[V] nicht mit einem zerosized type aufruft. Bitte den docstring dbzgl. noch verbessern |
|
Tip For best results, initiate chat on the files or code changes. @gaissmai: Absolut, guter Punkt! Hier ist der verbesserte docstring: // Package value provides utilities for working with generic type parameters
// at runtime.
//
// The primary functionality is zero-sized type (ZST) detection via IsZST[V].
// This is critical for runtime validation: Fast[V] cannot work correctly with
// zero-sized types (like struct{} or [0]byte) and must be rejected at
// initialization. IsZST enables this safety check.
//
// Additionally, ZST detection optimizes dumping and printing operations by
// avoiding unnecessary storage and marshaling overhead for types that occupy
// no memory.
//
// This is an internal package used by the bart data structure implementation.
package valueDie wichtigsten Änderungen:
|
|
@coderabbitai seh schön, gefällt mir, bis auf den text bzgl. dumping und printing. Es geht mir nicht um overhead oder speicherverbrauch sondern um eine präzisere Darstellung im dump und im print, denn zero-sized types tragen keine Information für values und machen nur line noise und erschweren den dump zu lesen |
|
Tip For best results, initiate chat on the files or code changes. @gaissmai: Perfekt, verstehe! Hier ist der überarbeitete docstring: // Package value provides utilities for working with generic type parameters
// at runtime.
//
// The primary functionality is zero-sized type (ZST) detection via IsZST[V].
// This is critical for runtime validation: Fast[V] cannot work correctly with
// zero-sized types (like struct{} or [0]byte) and must be rejected at
// initialization. IsZST enables this safety check.
//
// Additionally, ZST detection improves the clarity of debug output. Since
// zero-sized types carry no information in their values, omitting them from
// dumps and prints reduces line noise and makes the output more readable.
//
// This is an internal package used by the bart data structure implementation.
package valueDie Änderungen:
✏️ Learnings added
|
|
@coderabbitai fast richtig. Fast[zero-sized type] wird nicht bei der Initialisierung rejected sondern bei Insert und InsertPersist, siehe auch den code in fast.go |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10089 @gaissmai: Perfekt, du hast recht! Hier ist der korrigierte docstring: // Package value provides utilities for working with generic type parameters
// at runtime.
//
// The primary functionality is zero-sized type (ZST) detection via IsZST[V].
// This is critical for runtime validation: Fast[V] cannot work correctly with
// zero-sized types (like struct{} or [0]byte). IsZST enables a safety check
// that panics during Insert and InsertPersist operations when a zero-sized
// type is detected.
//
// Additionally, ZST detection improves the clarity of debug output. Since
// zero-sized types carry no information in their values, omitting them from
// dumps and prints reduces line noise and makes the output more readable.
//
// This is an internal package used by the bart data structure implementation.
package valueDie Änderung:
|
|
@coderabbitai sehr schön, jetzt gefällt es mir. Bitte hilf mir auch noch beim docstring von Equal. Es wäre gut wenn der docstring noch darauf hinweisen würde, dass wenn der payload V das Interface Equaler erfüllt, ein eventuelles reflext.DeepEqual vermieden werden kann |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 67 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 643 🏁 Script executed: Length of output: 1359 🏁 Script executed: Length of output: 1503 @gaissmai: Sehr gerne! Hier ist der verbesserte docstring für // Equal compares two values of type V for equality.
// If V implements Equaler[V], that custom equality method is used,
// avoiding the potentially expensive reflect.DeepEqual.
// Otherwise, reflect.DeepEqual is used as a fallback.
func Equal[V any](v1, v2 V) bool {Die Verbesserung:
Passt das so für dich? |
|
@coderabbitai das ist an der falschen Stelle. Ich meinte den docstring in der public API für Table.Equal, Fast.Equal und Lite.Equal |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2218 🏁 Script executed: Length of output: 58190 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 989 🏁 Script executed: Length of output: 40 🏁 Script executed: Length of output: 102 🏁 Script executed: Length of output: 410 🏁 Script executed: Length of output: 408 🏁 Script executed: Length of output: 936 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 47 🏁 Script executed: Length of output: 71 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 829 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 1030 🏁 Script executed: Length of output: 101 🏁 Script executed: Length of output: 710 🏁 Script executed: Length of output: 670 🏁 Script executed: Length of output: 288 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 391 🏁 Script executed: Length of output: 2597 🏁 Script executed: Length of output: 1968 🏁 Script executed: Length of output: 375 🏁 Script executed: Length of output: 11583 @gaissmai: Verstanden! Hier sind die verbesserten docstrings für die Equal-Methoden: Für // Equal checks whether two tables are structurally and semantically equal.
// It ensures both trees (IPv4-based and IPv6-based) have the same sizes and
// recursively compares their root nodes.
//
// Value comparisons use reflect.DeepEqual by default. To avoid the potentially
// expensive reflect.DeepEqual, the payload type V can implement the Equaler[V]
// interface to provide custom equality logic.
func (t *_TABLE_TYPE[V]) Equal(o *_TABLE_TYPE[V]) bool {Für // Equal checks whether two tables are structurally and semantically equal.
// It ensures both trees (IPv4-based and IPv6-based) have the same sizes and
// recursively compares their root nodes.
//
// Note: Lite has no payload values, so this only checks structural equality.
func (l *Lite) Equal(o *Lite) bool {Der wichtige Unterschied:
Nach dem Update müssen die generierten Files mit dem Script neu erzeugt werden: go generate ./... |
|
@coderabbitai gut gemacht, ich übernehme das in etwas abgewandelter Form, siehe meinen nächsten commit |
|
Tip For best results, initiate chat on the files or code changes. @gaissmai: Sehr gerne! Freut mich, dass ich helfen konnte. Ich bin gespannt auf deine angepasste Version im nächsten Commit. 👍 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/nodes/commontests_tmpl.go (1)
691-717: Missing t.Parallel() in TestFprintRec_ZST.The test logic correctly validates that ZST values are omitted from FprintRec output. However, this test is missing the
t.Parallel()call that is present in all other test functions in this file (includingTestFprintRec_NonZST__NODE_TYPEon line 721). This is an inconsistency that should be addressed.Apply this diff to add the missing parallel directive:
func TestFprintRec_ZST__NODE_TYPE(t *testing.T) { + t.Parallel() + node := new(_NODE_TYPE[struct{}])
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
bartmethodsgenerated.go(1 hunks)commonmethods_tmpl.go(1 hunks)fastmethodsgenerated.go(1 hunks)internal/nodes/commontests_tmpl.go(9 hunks)internal/nodes/nodebasics.go(1 hunks)internal/value/value.go(1 hunks)lite.go(1 hunks)litemethodsgenerated.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- bartmethodsgenerated.go
- fastmethodsgenerated.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/value/value.go
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-28T20:34:10.928Z
Learning: In the bart codebase, zero-sized types (ZSTs) are omitted from dump and print output not for performance reasons, but to improve readability. ZST values carry no information and create unnecessary line noise that makes debug output harder to read.
📚 Learning: 2025-10-05T21:46:51.386Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:46:51.386Z
Learning: In internal/nodes, FastNode and LiteNode expose DumpString(octets []uint8, depth int, is4 bool, printVals bool) string with identical traversal/error semantics as BartNode; tests must construct multi-level paths via InsertChild with node-typed children to avoid Fringe/Leaf along the traversal path, and assert the two error branches (kid not set, kid wrong type).
Applied to files:
internal/nodes/commontests_tmpl.go
📚 Learning: 2025-10-05T07:11:38.369Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T07:11:38.369Z
Learning: LiteNode in the bart codebase does not use the allot mechanism like FastNode does. LiteNode.Contains() checks for ancestor prefixes in the complete binary tree using LookupTbl, not descendants. When testing LiteNode, ensure tests reflect ancestor relationships (idx, idx>>1, idx>>2, ...) rather than descendant propagation (idx<<1, (idx<<1)+1, ...).
Applied to files:
internal/nodes/commontests_tmpl.go
📚 Learning: 2025-10-05T07:12:24.637Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T07:12:24.637Z
Learning: LiteNode in the bart codebase does not use the allot mechanism like FastNode does. LiteNode.Contains() checks for ancestor prefixes in the complete binary tree using LookupTbl[idx], not descendants. When testing LiteNode, ensure tests reflect ancestor relationships (idx, idx>>1, idx>>2, ...) rather than descendant propagation. The allot table mentioned by the maintainer (32→64,65,128,...) applies to FastNode's descendant propagation, not LiteNode's ancestor checking.
Applied to files:
internal/nodes/commontests_tmpl.go
📚 Learning: 2025-10-05T19:56:18.121Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T19:56:18.121Z
Learning: In internal/nodes LiteNode does not store values; tests for Modify/Insert/Get must assert existence (ok) rather than value equality. Prefer Get(pfx) for verification and avoid asserting returned values.
Applied to files:
internal/nodes/commontests_tmpl.go
📚 Learning: 2025-10-19T06:22:03.283Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-19T06:22:03.283Z
Learning: Tests that verify modified payload values should skip liteTable because liteTable uses struct{} as its payload type and has no real payload to verify modifications on.
Applied to files:
internal/nodes/commontests_tmpl.go
📚 Learning: 2025-10-28T20:34:10.928Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-28T20:34:10.928Z
Learning: In the bart codebase, zero-sized types (ZSTs) are omitted from dump and print output not for performance reasons, but to improve readability. ZST values carry no information and create unnecessary line noise that makes debug output harder to read.
Applied to files:
internal/nodes/commontests_tmpl.go
📚 Learning: 2025-10-05T21:26:24.231Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:26:24.231Z
Learning: BartNode DumpString signature in internal/nodes: DumpString(octets []uint8, depth int, is4 bool, printVals bool) string. It traverses down the child path (expecting *BartNode at each step) before dumping the subtree with Dump. Tests must pass octets (first N octets) and a matching depth to target deeper nodes.
Applied to files:
internal/nodes/commontests_tmpl.go
🧬 Code graph analysis (1)
internal/nodes/commontests_tmpl.go (2)
internal/nodes/nodebasics.go (2)
StridePath(33-33)TrieItem(37-48)internal/nodes/lite.go (1)
LiteNode(62-69)
🔇 Additional comments (9)
lite.go (1)
395-396: LGTM! Clear documentation improvement.The added note accurately clarifies that
Lite.Equalonly checks structural equality sinceLitehas no payload values (it usesstruct{}as the type parameter). This helpful distinction makes it clear to users that unlikeTable.EqualorFast.Equal, this method doesn't compare payload values.litemethodsgenerated.go (1)
477-480: Documentation improvement clarifies equality semantics.The updated documentation clearly explains that value comparisons use
reflect.DeepEqualby default and that implementingEqualer[V]provides a custom equality path. This is a helpful clarification for users of the API.commonmethods_tmpl.go (1)
538-541: Template documentation matches generated output.The Equal documentation in this template correctly matches the generated code in
litemethodsgenerated.go. This ensures consistency across all generated table types.internal/nodes/nodebasics.go (1)
341-344: Consistent Equal documentation across the codebase.The updated documentation aligns with the Equal method documentation in
commonmethods_tmpl.goandlitemethodsgenerated.go, providing a consistent explanation of the equality logic throughout the codebase.internal/nodes/commontests_tmpl.go (5)
16-16: Necessary imports and method signatures for new tests.The
bytesimport and new method signatures (dump,FprintRec) are correctly added to support the new ZST and non-ZST test functions introduced later in the file.Also applies to: 32-32, 34-34
86-86: Systematic test function renaming for code generation.The test functions have been consistently renamed from single underscore to double underscore (e.g.,
TestInsertDelete_NODE_TYPE→TestInsertDelete__NODE_TYPE). This appears to be a systematic change related to the code generation templating system.Also applies to: 373-373, 423-423, 456-456, 489-489, 563-563
637-657: ZST dump test correctly validates omission of values.This test properly verifies that when the value type is zero-sized (
struct{}), thedumpoutput omits thevalues()section. This aligns with the learning that ZSTs are omitted from dump output to improve readability.Based on learnings
660-688: Non-ZST dump test validates value printing.This test correctly verifies that non-zero-sized types (e.g.,
int) include thevalues()section and actual values in dump output. The LiteNode skip logic is correct, skipping when the node IS a LiteNode (which has no real payload).Based on learnings
720-755: Non-ZST FprintRec test validates value printing.This test correctly verifies that non-zero-sized types (e.g.,
string) include values in FprintRec output. The LiteNode skip logic is correct, and the test properly includest.Parallel().Based on learnings
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
internal/nodes/fast.go (1)
334-351: Bug: taking address of range var (&kidAny) corrupts children; breakage of required cell sharing
kidAnyis reused per-iteration; storing&kidAnymakes all entries point to the same cell and does not preserve the original*anycell sharing. Use the original pointer cell fromn.Apply:
- case *FastNode[V]: - // link with new *any pointer - c.Children.Items[addr] = &kidAny + case *FastNode[V]: + // share the original *any cell to preserve allot/CBT semantics and avoid range-var address bug + c.Children.Items[addr] = n.Children.Items[addr]internal/nodes/litemethodsgenerated.go (1)
1427-1437: Persist COW: handleMatrixPersist must not mutate existing Leaf/FringeSwap in a new child with cloned value for the leaf+leaf(same) and fringe+fringe paths.
@@ - if thisIsFringe && otherIsFringe { - thisFringe.Value = cloneFn(otherFringe.Value) - return 1 - } + if thisIsFringe && otherIsFringe { + n.InsertChild(addr, &FringeNode[V]{Value: cloneFn(otherFringe.Value)}) + return 1 + } @@ - if thisIsLeaf && otherIsLeaf && thisLeaf.Prefix == otherLeaf.Prefix { - thisLeaf.Value = cloneFn(otherLeaf.Value) - return 1 - } + if thisIsLeaf && otherIsLeaf && thisLeaf.Prefix == otherLeaf.Prefix { + n.InsertChild(addr, &LeafNode[V]{Prefix: thisLeaf.Prefix, Value: cloneFn(otherLeaf.Value)}) + return 1 + }Also applies to: 1456-1466
internal/nodes/bartmethodsgenerated.go (1)
1427-1437: Persist COW: handleMatrixPersist must not mutate existing Leaf/FringeReplace child with a fresh node when overwriting values in the leaf+leaf(same) and fringe+fringe fast paths.
@@ - if thisIsFringe && otherIsFringe { - thisFringe.Value = cloneFn(otherFringe.Value) - return 1 - } + if thisIsFringe && otherIsFringe { + n.InsertChild(addr, &FringeNode[V]{Value: cloneFn(otherFringe.Value)}) + return 1 + } @@ - if thisIsLeaf && otherIsLeaf && thisLeaf.Prefix == otherLeaf.Prefix { - thisLeaf.Value = cloneFn(otherLeaf.Value) - return 1 - } + if thisIsLeaf && otherIsLeaf && thisLeaf.Prefix == otherLeaf.Prefix { + n.InsertChild(addr, &LeafNode[V]{Prefix: thisLeaf.Prefix, Value: cloneFn(otherLeaf.Value)}) + return 1 + }Also applies to: 1456-1466
internal/nodes/commonmethods_tmpl.go (1)
1461-1467: Persist COW violation: handle leaf+leaf and fringe+fringe cases safelyIn
handleMatrixPersist, the fringe+fringe and leaf+leaf cases directly mutatethisLeaf.ValueandthisFringe.Value(lines 1491, 1500). These children come fromn.GetChild(addr)and are not cloned first, so mutations corrupt the original node if it's shared. Replace mutations withInsertChildto create new isolated nodes:- // Special case: fringe + fringe -> just overwrite value - if thisIsFringe && otherIsFringe { - thisFringe.Value = cloneFn(otherFringe.Value) - return 1 - } + // Special case: fringe + fringe -> replace child with cloned value + if thisIsFringe && otherIsFringe { + n.InsertChild(addr, &FringeNode[V]{Value: cloneFn(otherFringe.Value)}) + return 1 + }- // Special case: leaf + leaf with same prefix -> just overwrite value - if thisIsLeaf && otherIsLeaf && thisLeaf.Prefix == otherLeaf.Prefix { - thisLeaf.Value = cloneFn(otherLeaf.Value) - return 1 - } + // Special case: leaf + leaf with same prefix -> replace child with cloned value + if thisIsLeaf && otherIsLeaf && thisLeaf.Prefix == otherLeaf.Prefix { + n.InsertChild(addr, &LeafNode[V]{Prefix: thisLeaf.Prefix, Value: cloneFn(otherLeaf.Value)}) + return 1 + }
🧹 Nitpick comments (7)
internal/nodes/commontests_tmpl.go (3)
37-38: Align stub signatures with internal/value.CloneFunc to avoid IDE drift.These generator stubs still reference CloneFunc without the value qualifier. Updating them keeps gopls/dev ergonomics consistent with the refactor.
@@ -import ( - "bytes" - "io" - "iter" - "math/rand/v2" - "net/netip" - "slices" - "strings" - "testing" - - "github.com/gaissmai/bart/internal/tests/golden" - "github.com/gaissmai/bart/internal/tests/random" -) +import ( + "bytes" + "io" + "iter" + "math/rand/v2" + "net/netip" + "slices" + "strings" + "testing" + + "github.com/gaissmai/bart/internal/tests/golden" + "github.com/gaissmai/bart/internal/tests/random" + "github.com/gaissmai/bart/internal/value" +) @@ -func (n *_NODE_TYPE[V]) InsertPersist(CloneFunc[V], netip.Prefix, V, int) (_ bool) { return } -func (n *_NODE_TYPE[V]) DeletePersist(CloneFunc[V], netip.Prefix) (_ bool) { return } +func (n *_NODE_TYPE[V]) InsertPersist(value.CloneFunc[V], netip.Prefix, V, int) (_ bool) { return } +func (n *_NODE_TYPE[V]) DeletePersist(value.CloneFunc[V], netip.Prefix) (_ bool) { return }Also applies to: 15-27
653-656: Strengthen ZST dump check: also assert prefixes section is present.Guard against false negatives by ensuring “prefixes(” exists while “values(” is absent.
output := buf.String() - // For ZST, dump should print prefxs(#N) but skip the "values(#N):" section + // For ZST, dump should print prefixes(#N) but skip the "values(#N):" section + if !strings.Contains(output, "prefixes(") { + t.Errorf("Expected 'prefixes()' section for ZST, but not found in:\n%s", output) + } if strings.Contains(output, "values(") { t.Errorf("Expected no 'values()' section for ZST, but found it in:\n%s", output) }Also applies to: 651-656
665-668: Non‑ZST dump: assert the CIDR line exists too.Add an explicit check for the target prefix to reduce accidental passes.
// For non-ZST, dump should include the "values(#N):" section if !strings.Contains(output, "values(") { t.Errorf("Expected 'values()' section for non-ZST, but not found in:\n%s", output) } // Should contain the actual value if !strings.Contains(output, "42") { t.Errorf("Expected value '42' in output, but not found in:\n%s", output) } + // And the CIDR itself + if !strings.Contains(output, "10.0.0.0/7") { + t.Errorf("Expected CIDR '10.0.0.0/7' in output, but not found in:\n%s", output) + }Also applies to: 679-687, 670-676
internal/nodes/lite.go (1)
43-50: Doc nit: use canonical type names (BartNode/FastNode).Minor wording cleanup for clarity.
-// LiteNode[V] satisfy the generic interface nodeReadWriter[V]. -// This allows LiteNode, fastNode, and node to be interchangeable under the +// LiteNode[V] satisfy the generic interface nodeReadWriter[V]. +// This allows LiteNode, FastNode, and BartNode to be interchangeable under the // same interface abstraction, enabling generic algorithms for insertion, -// lookup, dumping, and traversal, regardless of the internal representation. +// lookup, dumping, and traversal, regardless of the internal representation.zz-example_table_concurrent_test.go (1)
13-19: Test-local Cloner interface is redundantTable detects Clone() by method shape; this extra interface in bart_test isn’t needed and may confuse with bart.Cloner. Consider removing it or renaming to avoid implying it’s wired into the API.
internal/nodes/fastmethodsgenerated.go (2)
1259-1289: Union helpers: defaulting nil cloneFn and cloning ‘other’Behavior is correct. One nit: in handleMatrixPersist’s “other is node” branch when
thisChildis non-node and a freshncis created, you callnc.UnionRec(...). It’s safe (nc is new) but inconsistent with the “Persist” naming. ConsiderUnionRecPersistfor readability.- return nc.UnionRec(cloneFn, otherNode, depth+1) + return nc.UnionRecPersist(cloneFn, otherNode, depth+1)Also applies to: 1309-1406, 1427-1531
817-823: Typo: “indexs” → “indices”Minor wording polish in dump output.
- fmt.Fprintf(w, "%sindexs(#%d): %v\n", indent, nPfxCount, allIndices) + fmt.Fprintf(w, "%sindices(#%d): %v\n", indent, nPfxCount, allIndices)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
common.go(4 hunks)internal/nodes/bart.go(3 hunks)internal/nodes/bartmethodsgenerated.go(18 hunks)internal/nodes/commonmethods_tmpl.go(18 hunks)internal/nodes/commontests_tmpl.go(9 hunks)internal/nodes/fast.go(3 hunks)internal/nodes/fastmethodsgenerated.go(18 hunks)internal/nodes/lite.go(3 hunks)internal/nodes/litemethodsgenerated.go(18 hunks)internal/nodes/nodebasics.go(3 hunks)internal/nodes/nodebasics_test.go(3 hunks)internal/value/value.go(1 hunks)zz-example_fast_concurrent_test.go(1 hunks)zz-example_table_concurrent_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (23)
📚 Learning: 2025-09-12T20:44:35.654Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-12T20:44:35.654Z
Learning: In this codebase, Cloner[V] is applied during structural cloning of persistent tables (cloneFlat with cloneFn) and not at the moment a brand-new value is inserted. InsertPersist/ModifyPersist store new values as-is; cloning occurs when existing values are carried into a new persistent table. Modify/ModifyPersist return oldVal on updates, newVal on inserts, with deleted indicating deletion.
Applied to files:
zz-example_table_concurrent_test.gointernal/nodes/commonmethods_tmpl.gointernal/nodes/fastmethodsgenerated.gointernal/nodes/bartmethodsgenerated.gointernal/nodes/litemethodsgenerated.go
📚 Learning: 2025-09-23T14:26:38.307Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-23T14:26:38.307Z
Learning: In gaissmai/bart cloner.go, cloneFlat for bartNode[V], fastNode[V], and liteNode[V] accepts a cloneFn cloneFunc[V]; passing nil is intentional and means “clone structure only, do not clone values”. Only when cloneFn != nil are the node’s values deep-cloned. Do not flag nil cloneFn usage in cloneFlat call sites.
Applied to files:
internal/nodes/nodebasics_test.gointernal/nodes/fast.gointernal/nodes/bart.gointernal/nodes/lite.gozz-example_fast_concurrent_test.gointernal/nodes/bartmethodsgenerated.gocommon.gointernal/nodes/nodebasics.go
📚 Learning: 2025-09-09T14:41:43.277Z
Learnt from: gaissmai
PR: gaissmai/bart#284
File: cloner.go:28-30
Timestamp: 2025-09-09T14:41:43.277Z
Learning: In Go generics, you cannot use `val == nil` directly on an unconstrained type parameter V because nil comparison requires the type to be constrained to nullable types. The correct approach for nil checking with generics is to use type assertion with comma-ok pattern: `cloner, ok := any(val).(Cloner[V])` followed by checking both `ok` and whether `cloner == nil`.
Applied to files:
internal/nodes/nodebasics_test.gocommon.go
📚 Learning: 2025-09-20T07:24:33.035Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-20T07:24:33.035Z
Learning: In the cloning functionality for fastNode, sharing pointer-to-interface cells between original and clone is required and intentional for cloneFlat to work properly. This should not be flagged as an issue.
Applied to files:
internal/nodes/fast.go
📚 Learning: 2025-10-05T21:46:51.386Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:46:51.386Z
Learning: In internal/nodes, FastNode and LiteNode expose DumpString(octets []uint8, depth int, is4 bool, printVals bool) string with identical traversal/error semantics as BartNode; tests must construct multi-level paths via InsertChild with node-typed children to avoid Fringe/Leaf along the traversal path, and assert the two error branches (kid not set, kid wrong type).
Applied to files:
internal/nodes/commontests_tmpl.gointernal/nodes/commonmethods_tmpl.gointernal/nodes/fastmethodsgenerated.gointernal/nodes/bartmethodsgenerated.gointernal/nodes/litemethodsgenerated.go
📚 Learning: 2025-10-05T07:11:38.369Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T07:11:38.369Z
Learning: LiteNode in the bart codebase does not use the allot mechanism like FastNode does. LiteNode.Contains() checks for ancestor prefixes in the complete binary tree using LookupTbl, not descendants. When testing LiteNode, ensure tests reflect ancestor relationships (idx, idx>>1, idx>>2, ...) rather than descendant propagation (idx<<1, (idx<<1)+1, ...).
Applied to files:
internal/nodes/commontests_tmpl.go
📚 Learning: 2025-10-05T07:12:24.637Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T07:12:24.637Z
Learning: LiteNode in the bart codebase does not use the allot mechanism like FastNode does. LiteNode.Contains() checks for ancestor prefixes in the complete binary tree using LookupTbl[idx], not descendants. When testing LiteNode, ensure tests reflect ancestor relationships (idx, idx>>1, idx>>2, ...) rather than descendant propagation. The allot table mentioned by the maintainer (32→64,65,128,...) applies to FastNode's descendant propagation, not LiteNode's ancestor checking.
Applied to files:
internal/nodes/commontests_tmpl.go
📚 Learning: 2025-10-05T19:56:18.121Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T19:56:18.121Z
Learning: In internal/nodes LiteNode does not store values; tests for Modify/Insert/Get must assert existence (ok) rather than value equality. Prefer Get(pfx) for verification and avoid asserting returned values.
Applied to files:
internal/nodes/commontests_tmpl.go
📚 Learning: 2025-10-19T06:22:03.283Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-19T06:22:03.283Z
Learning: Tests that verify modified payload values should skip liteTable because liteTable uses struct{} as its payload type and has no real payload to verify modifications on.
Applied to files:
internal/nodes/commontests_tmpl.go
📚 Learning: 2025-10-28T20:34:10.928Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-28T20:34:10.928Z
Learning: In the bart codebase, zero-sized types (ZSTs) are omitted from dump and print output not for performance reasons, but to improve readability. ZST values carry no information and create unnecessary line noise that makes debug output harder to read.
Applied to files:
internal/nodes/commontests_tmpl.go
📚 Learning: 2025-10-05T21:26:24.231Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:26:24.231Z
Learning: BartNode DumpString signature in internal/nodes: DumpString(octets []uint8, depth int, is4 bool, printVals bool) string. It traverses down the child path (expecting *BartNode at each step) before dumping the subtree with Dump. Tests must pass octets (first N octets) and a matching depth to target deeper nodes.
Applied to files:
internal/nodes/commontests_tmpl.gointernal/nodes/commonmethods_tmpl.gointernal/nodes/fastmethodsgenerated.gointernal/nodes/bartmethodsgenerated.gointernal/nodes/litemethodsgenerated.go
📚 Learning: 2025-09-20T22:32:23.233Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-20T22:32:23.233Z
Learning: All three BART node types (bartNode, liteNode, fastNode) achieve O(1) lookup performance through precomputed lookup tables (lmp.LookupTbl[idx]) and BitSet256 intersection operations, not O(log n) as might be expected from "sparse" data structures.
Applied to files:
internal/nodes/bart.go
📚 Learning: 2025-09-23T11:56:58.877Z
Learnt from: gaissmai
PR: gaissmai/bart#317
File: nodeunion_tmpl.go:426-430
Timestamp: 2025-09-23T11:56:58.877Z
Learning: In nodeunion_tmpl.go's unionRecPersist method, fringe nodes (thisKid) are already properly cloned before reaching the fringe/fringe case, so direct mutation of thisKid.value is safe and does not cause persistent aliasing issues.
Applied to files:
internal/nodes/commonmethods_tmpl.gointernal/nodes/fastmethodsgenerated.gointernal/nodes/bartmethodsgenerated.gointernal/nodes/litemethodsgenerated.go
📚 Learning: 2025-09-12T06:23:05.913Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-12T06:23:05.913Z
Learning: The leafNode[V] struct in this codebase has two fields: `prefix netip.Prefix` and `value V`. The prefix field is of type netip.Prefix, not []byte. When generating test code for leafNode, use netip.MustParsePrefix() or similar functions to create valid netip.Prefix values.
Applied to files:
internal/nodes/commonmethods_tmpl.gointernal/nodes/fastmethodsgenerated.gointernal/nodes/bartmethodsgenerated.gointernal/nodes/litemethodsgenerated.go
📚 Learning: 2025-10-05T21:27:32.948Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:27:32.948Z
Learning: BartNode DumpString signature in internal/nodes: func (n *BartNode[V]) DumpString(octets []uint8, depth int, is4 bool, printVals bool) string. It traverses down the child chain using GetChild(path[i]) and requires BartNode children along the octet/depth path; otherwise it returns an "ERROR: kid ..." string. Tests must ensure traversal paths are BartNode nodes (e.g., by inserting BartNode children via InsertChild) instead of Fringe/Leaf children.
Applied to files:
internal/nodes/commonmethods_tmpl.gointernal/nodes/fastmethodsgenerated.gointernal/nodes/bartmethodsgenerated.gointernal/nodes/litemethodsgenerated.go
📚 Learning: 2025-09-29T07:02:34.949Z
Learnt from: gaissmai
PR: gaissmai/bart#323
File: litetablemethods_gen.go:66-103
Timestamp: 2025-09-29T07:02:34.949Z
Learning: In litebasics_gen.go's insertPersist method, there is a copy-on-write bug in the leafNode case where kid.value = val directly mutates the shared leafNode without cloning it first, violating COW semantics. The liteNode case correctly clones with kid.cloneFlat(cloneFn) before mutation.
Applied to files:
internal/nodes/lite.gointernal/nodes/litemethodsgenerated.go
📚 Learning: 2025-10-07T17:43:21.478Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-07T17:43:21.478Z
Learning: liteNode in gaissmai/bart stores no values in LiteLeafNode (32 bytes: just netip.Prefix with 24B Addr + 1B uint8 + 7B padding) and LiteFringeNode (0 bytes payload, prefix implicit). This makes liteNode more memory-efficient than bartNode/fastNode which store values in their LeafNode (40 bytes: 8B value + 32B prefix) and FringeNode (8 bytes). When calculating bytes per prefix, divide total bytes by (prefixes + leaves + fringes).
Applied to files:
internal/nodes/lite.go
📚 Learning: 2025-10-17T18:36:43.515Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-17T18:36:43.515Z
Learning: In the bart package (all.go), the blank import of `internal/bitset` at the top level is essential for proper inlining of BitSet256 functions in production code. Without it, the Go inliner fails due to the deep nesting path (bart -> internal/nodes -> internal/sparse -> internal/bitset). This has been verified via CPU profiling.
Applied to files:
internal/nodes/bartmethodsgenerated.gocommon.go
📚 Learning: 2025-09-27T08:29:25.765Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-27T08:29:25.765Z
Learning: In the bart project, template files like nodeiterators_tmpl.go contain stub methods that are only needed for IDE and gopls support. The actual implementations are generated into specific files (bartnode.go, litenode.go, fastnode.go) with type-specific optimizations for performance. Methods like contains() cannot have the same implementation across all node types due to performance requirements.
Applied to files:
internal/nodes/bartmethodsgenerated.go
📚 Learning: 2025-10-05T21:31:48.381Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:31:48.381Z
Learning: In internal/nodes BartNode, inserting prefixes like /8, /16, /24 via Insert creates Fringe/Leaf, never normal BartNode nodes. For DumpString tests that must traverse deeper via octets/depth, construct the path with InsertChild to attach BartNode children; populate those nodes via InsertPrefix so the dump has content.
Applied to files:
internal/nodes/bartmethodsgenerated.go
📚 Learning: 2025-09-13T19:12:00.916Z
Learnt from: gaissmai
PR: gaissmai/bart#291
File: internal/sparse/array256.go:53-56
Timestamp: 2025-09-13T19:12:00.916Z
Learning: In the gaissmai/bart project, Array256[T] intentionally embeds bitset.BitSet256 as a public field to enable direct, high-performance bitset operations in critical code paths. Examples include direct bitset comparisons in equal.go (lines 37, 41) and direct bitset intersection/union operations in overlaps.go (lines 84, 98, 189, 192, 203, 366, 373). The embedded field design allows zero-cost abstraction for bitset operations and should never be refactored to a private field, as this would significantly hurt performance in hot paths.
Applied to files:
common.go
📚 Learning: 2025-09-13T19:12:00.916Z
Learnt from: gaissmai
PR: gaissmai/bart#291
File: internal/sparse/array256.go:53-56
Timestamp: 2025-09-13T19:12:00.916Z
Learning: In the gaissmai/bart project, the Array256[T] struct intentionally embeds bitset.BitSet256 as a public field to allow direct, efficient access to bitset operations in performance-critical code paths (e.g., in equal.go and overlaps.go). This design prioritizes performance over the safety concern of potential invariant-breaking mutations, and should not be suggested for refactoring.
Applied to files:
common.go
📚 Learning: 2025-09-29T18:27:16.452Z
Learnt from: gaissmai
PR: gaissmai/bart#324
File: nodecommon.go:28-30
Timestamp: 2025-09-29T18:27:16.452Z
Learning: In gaissmai/bart codebase using Go 1.24+, the stridePath type defined as [maxTreeDepth]uint8 works correctly with netip calls like netip.AddrFrom16(path) and netip.AddrFrom4([4]byte(path[:4])) without needing type aliases. Go 1.24+ supports these type conversions natively.
Applied to files:
internal/nodes/nodebasics.go
🧬 Code graph analysis (12)
internal/nodes/nodebasics_test.go (1)
internal/value/value.go (2)
CloneFunc(82-82)Cloner(76-78)
internal/nodes/fast.go (1)
internal/value/value.go (1)
CloneFunc(82-82)
internal/nodes/commontests_tmpl.go (2)
internal/nodes/nodebasics.go (2)
StridePath(33-33)TrieItem(37-48)internal/nodes/lite.go (1)
LiteNode(63-70)
internal/nodes/bart.go (1)
internal/value/value.go (1)
CloneFunc(82-82)
internal/nodes/commonmethods_tmpl.go (3)
internal/bitset/bitset256.go (1)
BitSet256(59-59)internal/value/value.go (4)
CloneFunc(82-82)Equal(63-70)IsZST(35-38)CopyVal(85-87)internal/nodes/nodebasics.go (1)
StridePath(33-33)
internal/nodes/lite.go (1)
internal/value/value.go (1)
CloneFunc(82-82)
zz-example_fast_concurrent_test.go (2)
internal/value/value.go (1)
Cloner(76-78)zz-example_table_concurrent_test.go (1)
Cloner(17-19)
internal/nodes/fastmethodsgenerated.go (2)
internal/nodes/fast.go (1)
FastNode(22-52)internal/value/value.go (4)
CloneFunc(82-82)Equal(63-70)IsZST(35-38)CopyVal(85-87)
internal/nodes/bartmethodsgenerated.go (2)
internal/nodes/bart.go (1)
BartNode(27-44)internal/value/value.go (4)
CloneFunc(82-82)Equal(63-70)IsZST(35-38)CopyVal(85-87)
common.go (1)
internal/value/value.go (3)
CloneFunc(82-82)Cloner(76-78)IsZST(35-38)
internal/nodes/nodebasics.go (1)
internal/value/value.go (1)
CloneFunc(82-82)
internal/nodes/litemethodsgenerated.go (1)
internal/value/value.go (4)
CloneFunc(82-82)Equal(63-70)IsZST(35-38)CopyVal(85-87)
🔇 Additional comments (23)
internal/nodes/commontests_tmpl.go (1)
665-668: LiteNode skip is correct.Skipping Dump_NonZST for LiteNode avoids asserting non-existent payloads (per learnings). LGTM.
common.go (1)
54-61: Migration to internal/value is correct and nil‑receiver safe.
- cloneFnFactory returns value.CloneFunc[V] only when V implements value.Cloner[V].
- cloneVal handles nil pointer receivers via the assertion and c == nil check.
- panicOnZST now delegates to value.IsZST as intended.
LGTM.
Also applies to: 69-74, 79-81
internal/nodes/lite.go (1)
216-234: Clone signatures moved to value.CloneFunc and Lite-specific semantics preserved.CloneFlat/CloneRec ignore cloneFn by design (no payloads). Implementation is consistent and efficient. LGTM.
Also applies to: 249-265
internal/nodes/nodebasics_test.go (1)
28-35: LGTM: tests migrated to value.CloneFunc/Cloner correctly.Type assertions and factories now use internal/value; nil Cloner handling remains correct.
Also applies to: 42-47
internal/nodes/bart.go (1)
195-236: Clone pipeline correctly applies value.CloneFunc to prefixes and recurses on BartNode children.
- Prefix values cloned only when cloneFn != nil.
- Leaf/Fringe nodes cloned via their helpers; BartNode children deep‑cloned in CloneRec.
- Switch default panic is appropriate for invariant enforcement.
LGTM.
Also applies to: 252-268
zz-example_fast_concurrent_test.go (1)
13-16: Review comment is incorrect and based on a false premise.The file
zz-example_fast_concurrent_test.godoes not define a localClonerinterface. Onlyzz-example_table_concurrent_test.godefines it (lines 13-16). The fast test file uses bareCloner[*testVal]on line 15, but these types are undefined in that file and would cause compilation errors.The real issue is that
zz-example_fast_concurrent_test.gois missing theClonerinterface andtestValtype definitions that exist in the table test file. The comment should remain as[bart.Cloner]until those definitions are added to this file, or alternatively, those definitions should be copied from the table test file.Additionally,
zz-example_table_concurrent_test.goline 50 has the inverse problem: it defines a localClonerinterface but the comment at that line still references[bart.Cloner]and should reference[Cloner]instead.Likely an incorrect or invalid review comment.
internal/nodes/fast.go (1)
356-376: CloneRec child replacement OKCloning subnodes into fresh
*anycells via localnodeAnyis correct and avoids aliasing.internal/value/value.go (2)
35-50: ZST detection approach LGTMHeap-escape + address compare with noinline is clear and avoids unsafe; acceptable trade-off here.
63-70: Equality hook is correct and minimalPreferring Equaler[V] before reflect.DeepEqual is the right default.
internal/nodes/nodebasics.go (2)
338-343: CloneLeaf integration with value.CloneFunc is soundNil -> copy, non-nil -> deep-clone; matches persist COW expectations.
348-353: CloneFringe integration LGTMSame semantics as CloneLeaf; consistent.
internal/nodes/fastmethodsgenerated.go (5)
114-200: InsertPersist signature/logic matches COW modelCloning internal nodes along the path and inserting new values as-is is correct per project semantics.
365-453: DeletePersist path-clone + purge/compress is consistentCOW on traversal, local mutations afterward; good.
714-717: Centralized equality via value.EqualGood move; honors custom Equaler[V] without scattering reflect usage.
Also applies to: 748-751, 760-763
803-809: Value printing gated by IsZSTSkips noise for ZST payloads; matches documented intent.
Also applies to: 831-843, 882-887, 901-907
1083-1105: FprintRec: ZST gate applied once per callEfficient and clear; avoids printing empty values.
zz-example_table_concurrent_test.go (1)
60-68: No action required—go.mod already specifies Go 1.24.0The range-over-int syntax (
for range 100_000andfor range 1_000) is valid in the codebase because go.mod already requires Go 1.24.0, which is well above the 1.22 minimum. The code is compliant.internal/nodes/commonmethods_tmpl.go (2)
836-842: ZST-aware printing: LGTMUsing value.IsZSTV to gate values in dump/FprintRec is clean and consistent.
Also applies to: 916-941, 1118-1139
148-148: Persist COW: avoid in-place mutation of Leaf/Fringe in InsertPersistIn the equal-prefix branches you set kid.Value = val on existing Leaf/Fringe. In persist paths this can alias prior tables. Replace the child with a new Leaf/Fringe instead of mutating in place. Based on learnings.
Apply:
@@ - case *LeafNode[V]: + case *LeafNode[V]: // reached a path compressed prefix // override value in slot if prefixes are equal if kid.Prefix == pfx { - kid.Value = val - // exists - return true + // persist: do not mutate shared leaf; replace child + n.InsertChild(octet, &LeafNode[V]{Prefix: kid.Prefix, Value: val}) + return true } @@ - case *FringeNode[V]: + case *FringeNode[V]: // reached a path compressed fringe // override value in slot if pfx is a fringe if IsFringe(depth, pfx) { - kid.Value = val - // exists - return true + // persist: do not mutate shared fringe; replace child + n.InsertChild(octet, &FringeNode[V]{Value: val}) + return true }Also applies to: 189-215
⛔ Skipped due to learnings
Learnt from: gaissmai PR: gaissmai/bart#323 File: litetablemethods_gen.go:66-103 Timestamp: 2025-09-29T07:02:34.949Z Learning: In litebasics_gen.go's insertPersist method, there is a copy-on-write bug in the leafNode case where kid.value = val directly mutates the shared leafNode without cloning it first, violating COW semantics. The liteNode case correctly clones with kid.cloneFlat(cloneFn) before mutation.Learnt from: gaissmai PR: gaissmai/bart#0 File: :0-0 Timestamp: 2025-09-28T18:23:30.744Z Learning: In persist purge/compress, the stack nodes have already been cloneFlat during the descent, so re-inserting or mutating leaf/fringe under those cloned parents is persistence-safe; only values are deep-cloned when cloneFn != nil.Learnt from: gaissmai PR: gaissmai/bart#317 File: nodeunion_tmpl.go:426-430 Timestamp: 2025-09-23T11:56:58.877Z Learning: In nodeunion_tmpl.go's unionRecPersist method, fringe nodes (thisKid) are already properly cloned before reaching the fringe/fringe case, so direct mutation of thisKid.value is safe and does not cause persistent aliasing issues.Learnt from: gaissmai PR: gaissmai/bart#0 File: :0-0 Timestamp: 2025-09-29T13:32:08.562Z Learning: In gaissmai/bart COW (Copy-on-Write) persistence operations, leaf nodes and fringe nodes are already cloned with cloneFlat() at a higher level during trie descent, making direct mutations at lower levels safe. Do not flag direct mutations on leafNode/fringeNode as COW violations when they occur within persist operations, as the nodes are already properly isolated.Learnt from: gaissmai PR: gaissmai/bart#0 File: :0-0 Timestamp: 2025-09-28T16:47:46.745Z Learning: Implement fastNode.modifyPersist with a one-pass, stack-based "clone on commit" scheme: - record frames during read-only descent - call cb once (clone old value only if cloneFn!=nil) - on no-op (missing && del=true) avoid all structural clones - replay frames applying cloneFlat along the path and mutate terminal slot via allot/deletePrefix - root replacement via "*top = *top.cloneFlat(cloneFn)" keeps external pointer stable.Learnt from: gaissmai PR: gaissmai/bart#0 File: :0-0 Timestamp: 2025-09-12T20:44:35.654Z Learning: In this codebase, Cloner[V] is applied during structural cloning of persistent tables (cloneFlat with cloneFn) and not at the moment a brand-new value is inserted. InsertPersist/ModifyPersist store new values as-is; cloning occurs when existing values are carried into a new persistent table. Modify/ModifyPersist return oldVal on updates, newVal on inserts, with deleted indicating deletion.Learnt from: gaissmai PR: gaissmai/bart#318 File: tablepersist.go:142-145 Timestamp: 2025-09-24T17:38:10.743Z Learning: In persistent operations like InsertPersist, when converting a fringeNode to a bartNode, the fringeNode (kid) has already been cloned as part of the parent node's cloneFlat() operation during descent, so kid.value is already properly isolated between tables and doesn't need additional cloning.internal/nodes/litemethodsgenerated.go (2)
803-809: ZST gating and value.Equal: LGTM
- printValues := !value.IsZSTV correctly suppresses values for ZSTs.
- value.Equal usage in EqualRec is appropriate.
Also applies to: 831-842, 882-907, 1083-1105
114-114: Persist COW: replace Leaf/Fringe instead of mutating in InsertPersistAvoid kid.Value = val on persist path; replace the child to prevent aliasing. Based on learnings.
@@ - case *LeafNode[V]: + case *LeafNode[V]: if kid.Prefix == pfx { - kid.Value = val - return true + n.InsertChild(octet, &LeafNode[V]{Prefix: kid.Prefix, Value: val}) + return true } @@ - case *FringeNode[V]: + case *FringeNode[V]: if IsFringe(depth, pfx) { - kid.Value = val - return true + n.InsertChild(octet, &FringeNode[V]{Value: val}) + return true }Also applies to: 155-162, 175-182
⛔ Skipped due to learnings
Learnt from: gaissmai PR: gaissmai/bart#323 File: litetablemethods_gen.go:66-103 Timestamp: 2025-09-29T07:02:34.949Z Learning: In litebasics_gen.go's insertPersist method, there is a copy-on-write bug in the leafNode case where kid.value = val directly mutates the shared leafNode without cloning it first, violating COW semantics. The liteNode case correctly clones with kid.cloneFlat(cloneFn) before mutation.Learnt from: gaissmai PR: gaissmai/bart#0 File: :0-0 Timestamp: 2025-09-29T13:32:08.562Z Learning: In gaissmai/bart COW (Copy-on-Write) persistence operations, leaf nodes and fringe nodes are already cloned with cloneFlat() at a higher level during trie descent, making direct mutations at lower levels safe. Do not flag direct mutations on leafNode/fringeNode as COW violations when they occur within persist operations, as the nodes are already properly isolated.Learnt from: gaissmai PR: gaissmai/bart#317 File: nodeunion_tmpl.go:426-430 Timestamp: 2025-09-23T11:56:58.877Z Learning: In nodeunion_tmpl.go's unionRecPersist method, fringe nodes (thisKid) are already properly cloned before reaching the fringe/fringe case, so direct mutation of thisKid.value is safe and does not cause persistent aliasing issues.Learnt from: gaissmai PR: gaissmai/bart#0 File: :0-0 Timestamp: 2025-09-28T18:23:30.744Z Learning: In persist purge/compress, the stack nodes have already been cloneFlat during the descent, so re-inserting or mutating leaf/fringe under those cloned parents is persistence-safe; only values are deep-cloned when cloneFn != nil.Learnt from: gaissmai PR: gaissmai/bart#0 File: :0-0 Timestamp: 2025-09-12T20:44:35.654Z Learning: In this codebase, Cloner[V] is applied during structural cloning of persistent tables (cloneFlat with cloneFn) and not at the moment a brand-new value is inserted. InsertPersist/ModifyPersist store new values as-is; cloning occurs when existing values are carried into a new persistent table. Modify/ModifyPersist return oldVal on updates, newVal on inserts, with deleted indicating deletion.Learnt from: gaissmai PR: gaissmai/bart#0 File: :0-0 Timestamp: 2025-09-28T16:47:46.745Z Learning: Implement fastNode.modifyPersist with a one-pass, stack-based "clone on commit" scheme: - record frames during read-only descent - call cb once (clone old value only if cloneFn!=nil) - on no-op (missing && del=true) avoid all structural clones - replay frames applying cloneFlat along the path and mutate terminal slot via allot/deletePrefix - root replacement via "*top = *top.cloneFlat(cloneFn)" keeps external pointer stable.Learnt from: gaissmai PR: gaissmai/bart#318 File: tablepersist.go:142-145 Timestamp: 2025-09-24T17:38:10.743Z Learning: In persistent operations like InsertPersist, when converting a fringeNode to a bartNode, the fringeNode (kid) has already been cloned as part of the parent node's cloneFlat() operation during descent, so kid.value is already properly isolated between tables and doesn't need additional cloning.internal/nodes/bartmethodsgenerated.go (2)
806-809: ZST gating, value.Equal, and default cloneFn: LGTM
- printValues := !value.IsZSTV and gated prints look correct.
- value.Equal in EqualRec is appropriate.
- Defaulting cloneFn to value.CopyVal in unions is reasonable.
Also applies to: 831-842, 882-907, 1083-1105, 712-715, 748-761, 1227-1229
114-114: Persist COW: replace Leaf/Fringe instead of mutating in InsertPersistDo not set kid.Value in place on persist path; replace the child at octet. Based on learnings.
@@ - case *LeafNode[V]: + case *LeafNode[V]: if kid.Prefix == pfx { - kid.Value = val - return true + n.InsertChild(octet, &LeafNode[V]{Prefix: kid.Prefix, Value: val}) + return true } @@ - case *FringeNode[V]: + case *FringeNode[V]: if IsFringe(depth, pfx) { - kid.Value = val - return true + n.InsertChild(octet, &FringeNode[V]{Value: val}) + return true }Also applies to: 155-162, 175-182
⛔ Skipped due to learnings
Learnt from: gaissmai PR: gaissmai/bart#323 File: litetablemethods_gen.go:66-103 Timestamp: 2025-09-29T07:02:34.949Z Learning: In litebasics_gen.go's insertPersist method, there is a copy-on-write bug in the leafNode case where kid.value = val directly mutates the shared leafNode without cloning it first, violating COW semantics. The liteNode case correctly clones with kid.cloneFlat(cloneFn) before mutation.Learnt from: gaissmai PR: gaissmai/bart#0 File: :0-0 Timestamp: 2025-09-29T13:32:08.562Z Learning: In gaissmai/bart COW (Copy-on-Write) persistence operations, leaf nodes and fringe nodes are already cloned with cloneFlat() at a higher level during trie descent, making direct mutations at lower levels safe. Do not flag direct mutations on leafNode/fringeNode as COW violations when they occur within persist operations, as the nodes are already properly isolated.Learnt from: gaissmai PR: gaissmai/bart#0 File: :0-0 Timestamp: 2025-09-28T18:23:30.744Z Learning: In persist purge/compress, the stack nodes have already been cloneFlat during the descent, so re-inserting or mutating leaf/fringe under those cloned parents is persistence-safe; only values are deep-cloned when cloneFn != nil.Learnt from: gaissmai PR: gaissmai/bart#317 File: nodeunion_tmpl.go:426-430 Timestamp: 2025-09-23T11:56:58.877Z Learning: In nodeunion_tmpl.go's unionRecPersist method, fringe nodes (thisKid) are already properly cloned before reaching the fringe/fringe case, so direct mutation of thisKid.value is safe and does not cause persistent aliasing issues.Learnt from: gaissmai PR: gaissmai/bart#0 File: :0-0 Timestamp: 2025-10-05T20:27:35.772Z Learning: BartNode insertion on empty node: pfxlen ≤ /7 -> stored in node Prefixes; pfxlen = /8 -> creates FringeNode child at first octet; pfxlen ≥ /16 -> creates LeafNode child (no intermediate node). Reflect this in tests’ structural expectations.Learnt from: gaissmai PR: gaissmai/bart#318 File: tablepersist.go:142-145 Timestamp: 2025-09-24T17:38:10.743Z Learning: In persistent operations like InsertPersist, when converting a fringeNode to a bartNode, the fringeNode (kid) has already been cloned as part of the parent node's cloneFlat() operation during descent, so kid.value is already properly isolated between tables and doesn't need additional cloning.Learnt from: gaissmai PR: gaissmai/bart#0 File: :0-0 Timestamp: 2025-09-28T16:47:46.745Z Learning: Implement fastNode.modifyPersist with a one-pass, stack-based "clone on commit" scheme: - record frames during read-only descent - call cb once (clone old value only if cloneFn!=nil) - on no-op (missing && del=true) avoid all structural clones - replay frames applying cloneFlat along the path and mutate terminal slot via allot/deletePrefix - root replacement via "*top = *top.cloneFlat(cloneFn)" keeps external pointer stable.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
internal/value/value_test.go (1)
38-67: Consider using subtests for better test output.The test logic is correct, but using
t.Runfor each test case would improve test output clarity and allow individual failures to be more easily identified.Apply this diff to add subtests:
func TestIsZeroSizedType(t *testing.T) { t.Parallel() tests := []struct { name string got bool want bool }{ { name: "struct{}", got: IsZST[struct{}](), want: true, }, { name: "[0]byte", got: IsZST[[0]byte](), want: true, }, { name: "int", got: IsZST[int](), want: false, }, } for _, tt := range tests { - if tt.got != tt.want { - t.Errorf("%s, want %v, got %v", tt.name, tt.want, tt.got) - } + t.Run(tt.name, func(t *testing.T) { + if tt.got != tt.want { + t.Errorf("want %v, got %v", tt.want, tt.got) + } + }) } }litemethodsgenerated.go (2)
61-66: Align docs with structural typing (avoid “Cloner interface” wording).Public docs now recommend implementing
Clone() Vvia structural typing. These method comments still say “Cloner interface,” which external users cannot import. Suggest rewording to the method-form to avoid confusion.Apply in all three spots (insertPersist/DeletePersist/Union). Example patch:
-// If the payload type V contains pointers or needs deep copying, -// it must implement the Cloner interface to support correct cloning. +// If V contains pointers or requires deep copying, implement: +// +// func (v V) Clone() V +// +// The bart package detects this at runtime via structural typing.Also applies to: 158-160, 417-419
83-84: Minor: centralize cloneFn creation (optional).
cloneFn := value.CloneFnFactory[V]()repeats across methods. Consider a small private helper on the receiver (or template-level helper) to reduce repetition. Low impact; keep if you prefer explicitness.fastmethodsgenerated.go (2)
61-63: Fix “Cloner interface” phrasing for consistency with public guidance.Use the method-form to avoid implying a public interface:
-// it must implement the Cloner interface to support correct cloning. +// implement: +// func (v V) Clone() V +// The bart package detects this at runtime via structural typing.Also applies to: 158-160, 417-419
83-84: Optional: factor out CloneFnFactory usage.As in lite, consider a tiny helper to avoid repetition. Low priority.
Also applies to: 186-187, 260-263, 426-427, 447-448, 525-526
bartmethodsgenerated.go (2)
61-63: Update cloning docs to method-based form.Replace “Cloner interface” with the
Clone() Vmethod guidance and structural-typing note (as proposed for other files).Also applies to: 158-160, 417-419
83-84: Optional dedup of CloneFnFactory calls.Same optional helper suggestion as in other variants.
Also applies to: 186-187, 260-263, 426-427, 447-448, 525-526
common.go (1)
24-38: Consider adding a short ZST note in package docs.Fast (and internal code paths) reject zero-sized payload types and panic on insert. A brief mention helps users avoid surprises.
Suggested insertion near the persistence paragraph:
// All variants also support copy-on-write persistence. +// +// Note: zero-sized payload types (e.g., struct{}) are not supported and +// inserts that would store such values will panic. Use a non-empty wrapper.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
bartmethodsgenerated.go(10 hunks)common.go(1 hunks)commonmethods_tmpl.go(10 hunks)fastmethodsgenerated.go(10 hunks)internal/nodes/fast.go(3 hunks)internal/nodes/lite.go(3 hunks)internal/nodes/nodebasics.go(3 hunks)internal/value/value_test.go(1 hunks)litemethodsgenerated.go(10 hunks)zz-example_custom_value_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (13)
📚 Learning: 2025-09-12T20:44:35.654Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-12T20:44:35.654Z
Learning: In this codebase, Cloner[V] is applied during structural cloning of persistent tables (cloneFlat with cloneFn) and not at the moment a brand-new value is inserted. InsertPersist/ModifyPersist store new values as-is; cloning occurs when existing values are carried into a new persistent table. Modify/ModifyPersist return oldVal on updates, newVal on inserts, with deleted indicating deletion.
Applied to files:
litemethodsgenerated.gofastmethodsgenerated.gobartmethodsgenerated.gocommonmethods_tmpl.go
📚 Learning: 2025-09-23T14:26:38.307Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-23T14:26:38.307Z
Learning: In gaissmai/bart cloner.go, cloneFlat for bartNode[V], fastNode[V], and liteNode[V] accepts a cloneFn cloneFunc[V]; passing nil is intentional and means “clone structure only, do not clone values”. Only when cloneFn != nil are the node’s values deep-cloned. Do not flag nil cloneFn usage in cloneFlat call sites.
Applied to files:
litemethodsgenerated.gointernal/nodes/fast.gofastmethodsgenerated.gobartmethodsgenerated.gointernal/nodes/nodebasics.gocommon.gointernal/nodes/lite.gocommonmethods_tmpl.go
📚 Learning: 2025-09-12T06:23:05.913Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-12T06:23:05.913Z
Learning: The leafNode[V] struct in this codebase has two fields: `prefix netip.Prefix` and `value V`. The prefix field is of type netip.Prefix, not []byte. When generating test code for leafNode, use netip.MustParsePrefix() or similar functions to create valid netip.Prefix values.
Applied to files:
litemethodsgenerated.gofastmethodsgenerated.gobartmethodsgenerated.gointernal/nodes/lite.gocommonmethods_tmpl.go
📚 Learning: 2025-09-09T14:41:43.277Z
Learnt from: gaissmai
PR: gaissmai/bart#284
File: cloner.go:28-30
Timestamp: 2025-09-09T14:41:43.277Z
Learning: In Go generics, you cannot use `val == nil` directly on an unconstrained type parameter V because nil comparison requires the type to be constrained to nullable types. The correct approach for nil checking with generics is to use type assertion with comma-ok pattern: `cloner, ok := any(val).(Cloner[V])` followed by checking both `ok` and whether `cloner == nil`.
Applied to files:
litemethodsgenerated.gofastmethodsgenerated.gobartmethodsgenerated.gocommonmethods_tmpl.go
📚 Learning: 2025-09-13T07:39:52.762Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-13T07:39:52.762Z
Learning: For bart routing table test files, use routeEntry struct with nextHop netip.Addr, exitIF string, and attributes map[string]int fields, with *routeEntry as the Fat[V] payload type in tests, instead of generic configuration objects. Implement Clone() method for deep cloning of the attributes map.
Applied to files:
litemethodsgenerated.gofastmethodsgenerated.gozz-example_custom_value_test.gocommon.gocommonmethods_tmpl.go
📚 Learning: 2025-09-13T07:30:12.421Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-13T07:30:12.421Z
Learning: For bart routing table documentation and examples, use routeEntry struct with nextHop netip.Addr, exitIF string, and attributes map[string]int fields, with *routeEntry as the Fat[V] payload type, instead of generic configuration objects.
Applied to files:
zz-example_custom_value_test.go
📚 Learning: 2025-09-29T18:27:16.452Z
Learnt from: gaissmai
PR: gaissmai/bart#324
File: nodecommon.go:28-30
Timestamp: 2025-09-29T18:27:16.452Z
Learning: In gaissmai/bart codebase using Go 1.24+, the stridePath type defined as [maxTreeDepth]uint8 works correctly with netip calls like netip.AddrFrom16(path) and netip.AddrFrom4([4]byte(path[:4])) without needing type aliases. Go 1.24+ supports these type conversions natively.
Applied to files:
internal/nodes/nodebasics.go
📚 Learning: 2025-10-05T19:56:18.121Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T19:56:18.121Z
Learning: In internal/nodes LiteNode does not store values; tests for Modify/Insert/Get must assert existence (ok) rather than value equality. Prefer Get(pfx) for verification and avoid asserting returned values.
Applied to files:
internal/nodes/lite.go
📚 Learning: 2025-10-07T17:43:21.478Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-07T17:43:21.478Z
Learning: liteNode in gaissmai/bart stores no values in LiteLeafNode (32 bytes: just netip.Prefix with 24B Addr + 1B uint8 + 7B padding) and LiteFringeNode (0 bytes payload, prefix implicit). This makes liteNode more memory-efficient than bartNode/fastNode which store values in their LeafNode (40 bytes: 8B value + 32B prefix) and FringeNode (8 bytes). When calculating bytes per prefix, divide total bytes by (prefixes + leaves + fringes).
Applied to files:
internal/nodes/lite.go
📚 Learning: 2025-10-05T07:12:24.637Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T07:12:24.637Z
Learning: LiteNode in the bart codebase does not use the allot mechanism like FastNode does. LiteNode.Contains() checks for ancestor prefixes in the complete binary tree using LookupTbl[idx], not descendants. When testing LiteNode, ensure tests reflect ancestor relationships (idx, idx>>1, idx>>2, ...) rather than descendant propagation. The allot table mentioned by the maintainer (32→64,65,128,...) applies to FastNode's descendant propagation, not LiteNode's ancestor checking.
Applied to files:
internal/nodes/lite.go
📚 Learning: 2025-10-05T07:11:38.369Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T07:11:38.369Z
Learning: LiteNode in the bart codebase does not use the allot mechanism like FastNode does. LiteNode.Contains() checks for ancestor prefixes in the complete binary tree using LookupTbl, not descendants. When testing LiteNode, ensure tests reflect ancestor relationships (idx, idx>>1, idx>>2, ...) rather than descendant propagation (idx<<1, (idx<<1)+1, ...).
Applied to files:
internal/nodes/lite.go
📚 Learning: 2025-09-29T07:02:34.949Z
Learnt from: gaissmai
PR: gaissmai/bart#323
File: litetablemethods_gen.go:66-103
Timestamp: 2025-09-29T07:02:34.949Z
Learning: In litebasics_gen.go's insertPersist method, there is a copy-on-write bug in the leafNode case where kid.value = val directly mutates the shared leafNode without cloning it first, violating COW semantics. The liteNode case correctly clones with kid.cloneFlat(cloneFn) before mutation.
Applied to files:
internal/nodes/lite.go
📚 Learning: 2025-09-17T13:38:33.273Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-17T13:38:33.273Z
Learning: The Lite type in the gaissmai/bart codebase is not generic and should be used as &Lite{} instead of &Lite[T]{}. The Lite.Insert() method only takes a prefix parameter, not a value parameter, as it only stores prefixes.
Applied to files:
internal/nodes/lite.go
🧬 Code graph analysis (9)
litemethodsgenerated.go (1)
internal/value/value.go (1)
CloneFnFactory(114-121)
internal/nodes/fast.go (2)
internal/bitset/bitset256.go (1)
BitSet256(59-59)internal/value/value.go (1)
CloneFunc(109-109)
fastmethodsgenerated.go (2)
internal/value/value.go (1)
CloneFnFactory(114-121)fast.go (1)
Fast(40-51)
zz-example_custom_value_test.go (1)
internal/value/value.go (1)
Equal(90-97)
bartmethodsgenerated.go (2)
internal/value/value.go (1)
CloneFnFactory(114-121)bart.go (1)
Table(31-42)
internal/nodes/nodebasics.go (1)
internal/value/value.go (1)
CloneFunc(109-109)
internal/value/value_test.go (1)
internal/value/value.go (6)
Equal(90-97)IsZST(54-57)PanicOnZST(73-77)CloneFnFactory(114-121)CloneVal(127-134)CopyVal(137-139)
internal/nodes/lite.go (3)
internal/sparse/array256.go (1)
Array256(47-50)internal/bitset/bitset256.go (1)
BitSet256(59-59)internal/value/value.go (1)
CloneFunc(109-109)
commonmethods_tmpl.go (1)
internal/value/value.go (1)
CloneFnFactory(114-121)
🔇 Additional comments (14)
internal/value/value_test.go (5)
12-36: LGTM! Well-designed test types.The test types effectively demonstrate both Equaler and Cloner interface implementations, with appropriate deep copy semantics for the clonableType using
maps.Clone.
69-107: LGTM! Proper panic testing with good coverage.The test correctly validates panic behavior for zero-sized types and ensures non-zero-sized types do not panic. Good use of subtests and parallel execution.
109-163: LGTM! Comprehensive equality testing.The test provides excellent coverage of the Equal function, validating both custom equality via the Equaler interface and the reflect.DeepEqual fallback for complex and simple types.
165-250: LGTM! Thorough clone testing with proper deep/shallow copy verification.The tests correctly validate both CloneFnFactory and CloneVal behavior. The deep copy verification (modifying cloned values should not affect originals) and shallow copy behavior (maps are shared when Cloner is not implemented) are properly tested.
252-295: LGTM! Correct validation of value copy semantics.The test properly validates CopyVal behavior across simple types, structs with reference fields, and pointers. The verification that maps and pointers are shared (shallow copy) correctly demonstrates Go's value copy semantics.
zz-example_custom_value_test.go (2)
10-24: LGTM! Clean implementation of custom value type.The Route type properly implements both Equaler[Route] and Cloner[Route] interfaces. The Equal method correctly compares both fields using
maps.Equalfor the Attrs map, and the Clone method performs a proper deep copy usingmaps.Clone.
26-35: LGTM! Clear and effective example.The example effectively demonstrates the custom value type usage with
Table[Route]. It shows that:
- InsertPersist uses the Route.Clone() method for persistence
- table.Clone() creates a properly cloned table
- table.Equal() correctly identifies cloned tables as equal using Route.Equal()
litemethodsgenerated.go (1)
478-491: Equal/Clone doc expansions look good.Clear guidance on
Equal(other V) boolandClone() Vwith structural typing. LGTM.Also applies to: 503-518
commonmethods_tmpl.go (1)
536-553: Equal/Clone doc blocks in the template are solid.They clearly document structural typing and examples. LGTM.
Also applies to: 564-579
fastmethodsgenerated.go (1)
478-491: Equal/Clone docs: LGTM.Also applies to: 503-518
bartmethodsgenerated.go (1)
478-491: Equal/Clone docs: LGTM.Also applies to: 503-518
internal/nodes/fast.go (1)
11-11: LGTM! Clean refactoring to use centralized value package.The migration from local
CloneFunc[V]tovalue.CloneFunc[V]is consistent and correct. The documentation improvements forFastNode[V](lines 16-35) provide excellent clarity on the node structure and memory optimizations.Also applies to: 306-306, 363-363
internal/nodes/lite.go (1)
12-12: LGTM! Consistent refactoring with appropriate phantom type handling.The signature updates to
value.CloneFunc[V]are correct, and the use of_prefix on the unused parameter appropriately signals thatLiteNodedoesn't clone values (it's a phantom type for interface satisfaction only).Also applies to: 183-183, 216-216
internal/nodes/nodebasics.go (1)
14-14: LGTM! Refactoring with added defensive nil checks.The migration to
value.CloneFunc[V]is consistent with the rest of the codebase. The new nil receiver checks (lines 339-341 and 353-355) are a good defensive programming practice that improves robustness without affecting existing behavior.Also applies to: 338-347, 352-360
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
fast.go (1)
61-71: Consider parity: add the same ZST guard to Fast.Modify.Modify can perform an insert when the prefix is absent; unlike ModifyPersist, it currently doesn’t pass through Insert/InsertPersist, so ZST payloads could slip in. Add the same once-guarded PanicOnZST to Fast.Modify (Fast-only). This likely needs a template tweak to gate the guard for Fast. I can sketch the template change if desired.
Also applies to: 248-248
commonmethods_tmpl.go (1)
309-343: Fast-only ZST guard hook for Modify/ModifyPersist (template-level approach).To ensure ZST is blocked for inserts done via Modify on Fast, add an optional hook in the template that calls a receiver method if present:
func (t *_TABLE_TYPE[V]) Modify(pfx netip.Prefix, cb func(_ V, ok bool) (_ V, del bool)) { + // Optional ZST guard: only implemented by Fast[V] + if z, ok := any(t).(interface{ panicOnZST() }); ok { + z.panicOnZST() + } if !pfx.IsValid() { return } ... }Then, in fast.go implement:
+func (t *Fast[V]) panicOnZST() { t.once.Do(value.PanicOnZST[V]) }This keeps Bart/Lite unaffected and aligns behavior across all Fast mutations.
fastmethodsgenerated.go (1)
248-282: Bridge the ZST gap for Modify on Fast (via template hook).Same as the template suggestion: add the optional panicOnZST() hook call in Modify here after regenerating, or implement a Fast-specific override. Prevents ZST insertion via Modify.
♻️ Duplicate comments (2)
commonmethods_tmpl.go (1)
122-124: Doc wording: prefer method-based structural typing over “Cloner interface”.Replace “implement the value.Cloner interface” with a method-based description (“implement Clone() V”), and note detection via structural typing. Regenerate afterward so all generated files reflect the change.
Apply this patch in the template (examples shown for all occurrences):
-// If the payload type V contains pointers or needs deep copying, -// it must implement the value.Cloner interface to support correct cloning. +// If V contains pointers or requires deep copying, implement: +// +// func (v V) Clone() V +// +// The bart package detects this at runtime via structural typing. -// Create a cloning function for deep copying values; -// returns nil if V does not implement the value.Cloner interface. +// Create a cloning function for deep copying values; +// returns nil if V does not implement a Clone() V method.After editing:
- Run: go generate ./...
- Verify no remaining “Cloner interface” mentions outside internal/value.
Also applies to: 142-145, 246-248, 485-487, 507-509
fastmethodsgenerated.go (1)
81-84: Generated docs still say “value.Cloner interface”; fix in template and regenerate.This file reflects the template wording; apply the template patch from commonmethods_tmpl.go and re-run go generate to update here.
Optional quick verification:
#!/bin/bash # Find lingering mentions after template change and regeneration rg -nP 'Cloner interface' -g '!internal/value/**'Also applies to: 184-187, 425-427, 446-448, 525-526
🧹 Nitpick comments (12)
common.go (1)
25-37: Consider enhancing the documentation with operational context.The documentation clearly explains the method signatures, but could be more helpful by including:
- When Clone() is called: Mention that persist methods (e.g., copy-on-write operations) use Clone() to create independent copies of values.
- When Equal() is used: Note that Equal() is used as an optimization to avoid
reflect.DeepEqualfor value comparisons.- Fallback behavior: Clarify what happens when these methods are not implemented (e.g., shallow copy for Clone,
reflect.DeepEqualfor Equal).- ZST restrictions: Based on the PR objectives, Fast[V] cannot work with zero-sized types (ZSTs) and will panic during Insert/InsertPersist. Consider documenting this constraint.
Example enhancement:
// -// For complex or pointer value types, you must provide custom deep cloning -// using Go's structural typing, implementing the following method on your -// value type V: +// For complex or pointer value types, provide custom deep cloning by +// implementing the following method on your value type V: // // func (v V) Clone() V // -// For custom Equality implement the following method on your value type V: +// Clone is called during persist operations (copy-on-write) to create +// independent copies. If not implemented, values are copied shallowly. +// +// For custom equality, implement the following method on your value type V: // // func (v V) Equal(other V) bool // -// The bart package detects the interface satisfaction automatically at -// runtime using type assertions. +// Equal is used as an optimization to avoid reflect.DeepEqual. If not +// implemented, reflect.DeepEqual is used as a fallback. +// +// The bart package detects interface satisfaction automatically at runtime +// using type assertions. +// +// Note: Fast[V] does not support zero-sized types (ZSTs) and will panic +// during Insert/InsertPersist if a ZST is detected.commonmethods_tmpl.go (1)
81-84: Nit: unify helper name in comments.Where you say “Create a cloning function…”, reference value.CloneFnFactoryV once and avoid repeating the “returns nil if … interface” wording (will be fixed by the doc patch above).
Also applies to: 145-145, 247-248, 321-322, 586-587
internal/nodes/fast.go (1)
303-359: Avoid taking the address of range variable (use new(any) for consistency).While Go ≥1.24 makes range vars per‑iteration, storing &kidAny is still non‑idiomatic and easy to misread. Prefer allocating a fresh *any, as elsewhere in InsertChild and for leaf/fringe cases.
- case *FastNode[V]: - // link with new *any pointer - c.Children.Items[addr] = &kidAny + case *FastNode[V]: + // link with new *any pointer (consistent and explicit) + p := new(any) + *p = kidAny + c.Children.Items[addr] = pinternal/nodes/lite.go (5)
71-71: Clarify the incomplete comment.The comment fragment on line 71 is unclear. Consider revising it to a complete sentence.
Apply this diff to improve clarity:
-// prefix is set at the given index. +// GetPrefix returns whether a prefix is set at the given index. +// Since LiteNode stores no values, the value return is always zero.
82-83: Clarify that values are zero for LiteNode.The comment states "Each iteration yields the prefix index (uint8) and its associated value (V)." For LiteNode, there are no associated values—only zero values returned for interface compatibility. This could mislead readers.
Apply this diff to clarify:
-// AllIndices returns an iterator over all prefix entries. -// Each iteration yields the prefix index (uint8) and its associated value (V). +// AllIndices returns an iterator over all prefix entries. +// Each iteration yields the prefix index (uint8) and a zero value of V +// (LiteNode stores no values; V is a phantom type for interface compatibility).
106-108: Ensure type parameter consistency in comment.The comment mentions "*LeafNode, or *FringeNode" without the type parameter. For consistency with the struct-level documentation (lines 24-26) which uses "*LeafNode[V]" and "*FringeNode[V]", consider adding the type parameter here as well.
Apply this diff:
-// InsertChild adds a child node at the specified address (0-255). -// The child can be a *LiteNode[V], *LeafNode, or *FringeNode. -// Returns true if a child already existed at that address. +// InsertChild adds a child node at the specified address (0-255). +// The child can be a *LiteNode[V], *LeafNode[V], or *FringeNode[V]. +// Returns true if a child already existed at that address.
174-174: Fix method name capitalization in comment.The comment references "lookupIdx" but should use the correct capitalized name "LookupIdx".
Apply this diff:
-// Lookup is just a simple wrapper for lookupIdx. +// Lookup is just a simple wrapper for LookupIdx.
77-79: Add clarifying comment to explain why LiteNode.MustGetPrefix doesn't panic.Verification confirms the concern is valid: BartNode and FastNode MustGetPrefix methods access sparse arrays and can panic, while LiteNode cannot because V is a phantom type with no underlying value storage. Although this is intentional interface compatibility, a clarifying comment would help maintainers understand the behavioral difference:
func (n *LiteNode[V]) MustGetPrefix(idx uint8) (_ V) { // Note: LiteNode stores no values (V is a phantom type). // Unlike BartNode and FastNode, this cannot panic even if idx is invalid. // Method exists for interface compatibility with common node operations. return }internal/value/value_test.go (4)
25-37: Add helper types for typed‑nil coverage (interface and pointer Cloner).These will enable precise tests for Equal on typed‑nil interfaces and for CloneVal on a nil pointer receiver implementing Cloner.
type nonClonableType struct { Data map[string]int } +// Helper interface/type to exercise typed‑nil-in-interface equality semantics. +type testIface interface{ M() } +type testImpl struct{} +func (*testImpl) M() {} + +// Helper Cloner with pointer receiver to exercise CloneVal on typed‑nil receivers. +type ptrClonable struct{} +func (p *ptrClonable) Clone() *ptrClonable { return p } // nil-in, nil-out
109-163: Add typed‑nil equality tests (interfaces, pointers, slices, maps).Covers tricky cases where an interface holds a typed‑nil pointer vs a nil interface, plus nil collections.
func TestEqual(t *testing.T) { t.Parallel() t.Run("with_Equaler_interface", func(t *testing.T) { @@ }) + t.Run("typed_nil_values", func(t *testing.T) { + t.Parallel() + // pointers + var p1 *int + var p2 *int + if !Equal[*int](p1, p2) { + t.Error("Equal should return true for two typed-nil pointers") + } + // slices + var s1 []int + var s2 []int + if !Equal(s1, s2) { + t.Error("Equal should return true for two nil slices") + } + // maps + var m1 map[string]int + var m2 map[string]int + if !Equal(m1, m2) { + t.Error("Equal should return true for two nil maps") + } + // interfaces containing typed nil vs nil interface + var i1 testIface = (*testImpl)(nil) + var i2 testIface = nil + if Equal[testIface](i1, i2) { + t.Error("Equal should return false for typed-nil-in-interface vs nil interface") + } + // interfaces containing typed nil vs typed nil (same dynamic type) + var i3 testIface = (*testImpl)(nil) + if !Equal[testIface](i1, i3) { + t.Error("Equal should return true for two typed-nil-in-interface values of same type") + } + }) }
206-250: Cover CloneVal on typed‑nil Cloner (pointer receiver).Exercises the nil‑receiver path and ensures no panic; also documents behavior when val is a typed‑nil.
func TestCloneVal(t *testing.T) { t.Parallel() @@ t.Run("simple_types", func(t *testing.T) { t.Parallel() original := 42 cloned := CloneVal(original) if original != cloned { t.Error("CloneVal should return same value for simple types") } }) + + t.Run("typed_nil_pointer_Cloner", func(t *testing.T) { + t.Parallel() + var original *ptrClonable // typed-nil + cloned := CloneVal(original) + if cloned != nil { + t.Error("CloneVal should return nil for typed-nil Cloner pointer") + } + }) }Note: In Go, an interface holding a typed‑nil pointer is non‑nil, so the
c == nilcheck in CloneVal (internal/value/value.go:126‑133) won’t short‑circuit; the method will be called on a nil receiver. The above test verifies this path is safe with a nil‑tolerant Clone method. Please confirm this matches the intended contract. Based on relevant snippet.
38-67: Optional: subtestify the IsZST table for clearer test output.Minor ergonomics improvement; keeps semantics identical and names each case.
func TestIsZeroSizedType(t *testing.T) { t.Parallel() - tests := []struct { + tests := []struct { name string got bool want bool }{ @@ - for _, tt := range tests { - if tt.got != tt.want { - t.Errorf("%s, want %v, got %v", tt.name, tt.want, tt.got) - } - } + for _, tt := range tests { + tt := tt // capture + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if tt.got != tt.want { + t.Fatalf("want %v, got %v", tt.want, tt.got) + } + }) + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
bart.go(1 hunks)bartmethodsgenerated.go(10 hunks)common.go(1 hunks)commonmethods_tmpl.go(10 hunks)fast.go(3 hunks)fastmethodsgenerated.go(10 hunks)internal/nodes/fast.go(3 hunks)internal/nodes/lite.go(3 hunks)internal/nodes/nodebasics.go(3 hunks)internal/value/value_test.go(1 hunks)litemethodsgenerated.go(10 hunks)zz-example_custom_value_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- bart.go
🚧 Files skipped from review as they are similar to previous changes (2)
- bartmethodsgenerated.go
- litemethodsgenerated.go
🧰 Additional context used
🧠 Learnings (13)
📚 Learning: 2025-09-12T20:44:35.654Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-12T20:44:35.654Z
Learning: In this codebase, Cloner[V] is applied during structural cloning of persistent tables (cloneFlat with cloneFn) and not at the moment a brand-new value is inserted. InsertPersist/ModifyPersist store new values as-is; cloning occurs when existing values are carried into a new persistent table. Modify/ModifyPersist return oldVal on updates, newVal on inserts, with deleted indicating deletion.
Applied to files:
fastmethodsgenerated.gocommonmethods_tmpl.gofast.go
📚 Learning: 2025-09-23T14:26:38.307Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-23T14:26:38.307Z
Learning: In gaissmai/bart cloner.go, cloneFlat for bartNode[V], fastNode[V], and liteNode[V] accepts a cloneFn cloneFunc[V]; passing nil is intentional and means “clone structure only, do not clone values”. Only when cloneFn != nil are the node’s values deep-cloned. Do not flag nil cloneFn usage in cloneFlat call sites.
Applied to files:
fastmethodsgenerated.gocommonmethods_tmpl.gointernal/nodes/fast.gocommon.gointernal/nodes/nodebasics.gointernal/nodes/lite.go
📚 Learning: 2025-09-09T14:41:43.277Z
Learnt from: gaissmai
PR: gaissmai/bart#284
File: cloner.go:28-30
Timestamp: 2025-09-09T14:41:43.277Z
Learning: In Go generics, you cannot use `val == nil` directly on an unconstrained type parameter V because nil comparison requires the type to be constrained to nullable types. The correct approach for nil checking with generics is to use type assertion with comma-ok pattern: `cloner, ok := any(val).(Cloner[V])` followed by checking both `ok` and whether `cloner == nil`.
Applied to files:
fastmethodsgenerated.gocommonmethods_tmpl.go
📚 Learning: 2025-09-13T07:39:52.762Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-13T07:39:52.762Z
Learning: For bart routing table test files, use routeEntry struct with nextHop netip.Addr, exitIF string, and attributes map[string]int fields, with *routeEntry as the Fat[V] payload type in tests, instead of generic configuration objects. Implement Clone() method for deep cloning of the attributes map.
Applied to files:
fastmethodsgenerated.gocommonmethods_tmpl.gocommon.gozz-example_custom_value_test.go
📚 Learning: 2025-09-29T18:27:16.452Z
Learnt from: gaissmai
PR: gaissmai/bart#324
File: nodecommon.go:28-30
Timestamp: 2025-09-29T18:27:16.452Z
Learning: In gaissmai/bart codebase using Go 1.24+, the stridePath type defined as [maxTreeDepth]uint8 works correctly with netip calls like netip.AddrFrom16(path) and netip.AddrFrom4([4]byte(path[:4])) without needing type aliases. Go 1.24+ supports these type conversions natively.
Applied to files:
internal/nodes/nodebasics.go
📚 Learning: 2025-10-05T19:56:18.121Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T19:56:18.121Z
Learning: In internal/nodes LiteNode does not store values; tests for Modify/Insert/Get must assert existence (ok) rather than value equality. Prefer Get(pfx) for verification and avoid asserting returned values.
Applied to files:
internal/nodes/lite.go
📚 Learning: 2025-10-05T07:11:38.369Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T07:11:38.369Z
Learning: LiteNode in the bart codebase does not use the allot mechanism like FastNode does. LiteNode.Contains() checks for ancestor prefixes in the complete binary tree using LookupTbl, not descendants. When testing LiteNode, ensure tests reflect ancestor relationships (idx, idx>>1, idx>>2, ...) rather than descendant propagation (idx<<1, (idx<<1)+1, ...).
Applied to files:
internal/nodes/lite.go
📚 Learning: 2025-10-05T07:12:24.637Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T07:12:24.637Z
Learning: LiteNode in the bart codebase does not use the allot mechanism like FastNode does. LiteNode.Contains() checks for ancestor prefixes in the complete binary tree using LookupTbl[idx], not descendants. When testing LiteNode, ensure tests reflect ancestor relationships (idx, idx>>1, idx>>2, ...) rather than descendant propagation. The allot table mentioned by the maintainer (32→64,65,128,...) applies to FastNode's descendant propagation, not LiteNode's ancestor checking.
Applied to files:
internal/nodes/lite.go
📚 Learning: 2025-09-17T13:38:33.273Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-17T13:38:33.273Z
Learning: The Lite type in the gaissmai/bart codebase is not generic and should be used as &Lite{} instead of &Lite[T]{}. The Lite.Insert() method only takes a prefix parameter, not a value parameter, as it only stores prefixes.
Applied to files:
internal/nodes/lite.go
📚 Learning: 2025-09-29T07:02:34.949Z
Learnt from: gaissmai
PR: gaissmai/bart#323
File: litetablemethods_gen.go:66-103
Timestamp: 2025-09-29T07:02:34.949Z
Learning: In litebasics_gen.go's insertPersist method, there is a copy-on-write bug in the leafNode case where kid.value = val directly mutates the shared leafNode without cloning it first, violating COW semantics. The liteNode case correctly clones with kid.cloneFlat(cloneFn) before mutation.
Applied to files:
internal/nodes/lite.go
📚 Learning: 2025-10-07T17:43:21.478Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-07T17:43:21.478Z
Learning: liteNode in gaissmai/bart stores no values in LiteLeafNode (32 bytes: just netip.Prefix with 24B Addr + 1B uint8 + 7B padding) and LiteFringeNode (0 bytes payload, prefix implicit). This makes liteNode more memory-efficient than bartNode/fastNode which store values in their LeafNode (40 bytes: 8B value + 32B prefix) and FringeNode (8 bytes). When calculating bytes per prefix, divide total bytes by (prefixes + leaves + fringes).
Applied to files:
internal/nodes/lite.go
📚 Learning: 2025-09-12T06:23:05.913Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-12T06:23:05.913Z
Learning: The leafNode[V] struct in this codebase has two fields: `prefix netip.Prefix` and `value V`. The prefix field is of type netip.Prefix, not []byte. When generating test code for leafNode, use netip.MustParsePrefix() or similar functions to create valid netip.Prefix values.
Applied to files:
internal/nodes/lite.go
📚 Learning: 2025-09-13T07:30:12.421Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-13T07:30:12.421Z
Learning: For bart routing table documentation and examples, use routeEntry struct with nextHop netip.Addr, exitIF string, and attributes map[string]int fields, with *routeEntry as the Fat[V] payload type, instead of generic configuration objects.
Applied to files:
zz-example_custom_value_test.go
🧬 Code graph analysis (8)
internal/value/value_test.go (1)
internal/value/value.go (6)
Equal(90-97)IsZST(54-57)PanicOnZST(73-77)CloneFnFactory(114-121)CloneVal(127-134)CopyVal(137-139)
fastmethodsgenerated.go (2)
internal/value/value.go (1)
CloneFnFactory(114-121)fast.go (1)
Fast(40-51)
commonmethods_tmpl.go (1)
internal/value/value.go (1)
CloneFnFactory(114-121)
internal/nodes/fast.go (2)
internal/bitset/bitset256.go (1)
BitSet256(59-59)internal/value/value.go (1)
CloneFunc(109-109)
internal/nodes/nodebasics.go (1)
internal/value/value.go (1)
CloneFunc(109-109)
fast.go (1)
internal/value/value.go (1)
PanicOnZST(73-77)
internal/nodes/lite.go (3)
internal/sparse/array256.go (1)
Array256(47-50)internal/bitset/bitset256.go (1)
BitSet256(59-59)internal/value/value.go (1)
CloneFunc(109-109)
zz-example_custom_value_test.go (2)
internal/value/value.go (1)
Equal(90-97)bart.go (1)
Table(31-42)
🔇 Additional comments (21)
zz-example_custom_value_test.go (4)
10-13: LGTM!The Route struct is well-suited for demonstrating custom value type features with both simple (ASN) and complex (Attrs map) fields that require deep copying and comparison.
15-17: LGTM!The Equal method correctly implements deep equality by comparing both the ASN field and the Attrs map using
maps.Equal. This implementation will be recognized by theinternal/valuepackage's Equaler[V] interface pattern.
19-24: LGTM!The Clone method correctly implements deep cloning by creating an independent copy of the Attrs map using
maps.Clone. This implementation will be recognized by theinternal/valuepackage's Cloner[V] interface pattern, enabling persist operations to safely store independent copies.
26-35: No action required—the example code is correct.The
mpphelper is properly defined inzz-example_test.goas a package-level variable (var mpp = netip.MustParsePrefix). Since bothzz-example_test.goandzz-example_custom_value_test.goare in the samebart_testpackage, themppfunction is accessible without explicit import or redeclaration. The example works as written.Likely an incorrect or invalid review comment.
fast.go (1)
12-13: Good move: centralize ZST guard via internal/value.Import and t.once.Do(value.PanicOnZST[V]) look correct and keep the hot path clean. No issues.
Also applies to: 67-70, 83-86
commonmethods_tmpl.go (4)
536-595: Clear, user-facing Clone docs.Clone docs now correctly show Clone() V and slices.Clone usage. Looks good.
539-553: Equal docs: good explanation of Equal(other V) and DeepEqual fallback.Accurately reflects value.Equal semantics.
320-323: Minor: avoid redundant Clone in ModifyPersist when ok==false.You already guard clone with (cloneFn != nil && ok). Good.
736-776: Dump/Fprint sections read clean; no behavior concerns.No issues; consistent with prior structure.
Also applies to: 813-897
fastmethodsgenerated.go (1)
413-433: Union/UnionPersist: cloneFn selection is correct; counters adjusted properly.Looks correct and efficient.
Also applies to: 435-473
internal/nodes/fast.go (4)
34-57: *Docs for Items [256]any and counters are clear.The memory rationale and cached counts are well explained.
111-127: InsertChild overwrites in-place and preserves counts—good.Reuse of existing *any slot reduces churn; count/bitset updates are correct.
361-381: CloneRec builds on CloneFlat correctly; targeted recursion on FastNode kids.Looks correct and avoids extra allocations.
145-165: Prefix pointer allotment and delete paths are sound.allot traversal and CBT indexing guard against overruns; delete backs off to parent pointer as intended.
Also applies to: 197-214, 256-301
internal/nodes/nodebasics.go (3)
338-347: Nil-safe CloneLeaf with value.CloneFunc—LGTM.Correctly handles nil receiver; applies deep clone only when provided.
352-361: Nil-safe CloneFringe—LGTM.Matches CloneLeaf semantics.
219-246: CIDR reconstruction helpers are precise; comments helpful.AddrFrom4/16 conversions and BCE masks are correct.
Also applies to: 259-283, 325-333
internal/nodes/lite.go (3)
12-12: LGTM!The import addition aligns with the refactor to centralize cloning utilities in the internal/value package.
15-29: LGTM!The documentation clearly describes the LiteNode structure and correctly notes that V is a phantom type used solely for common method generation.
183-201: LGTM!The CloneFlat and CloneRec signature changes correctly adopt value.CloneFunc[V] from the internal/value package. The implementations properly handle structural cloning, and passing nil for cloneFn is appropriate since LiteNode stores no values (V is a phantom type).
Also applies to: 216-232
internal/value/value_test.go (1)
6-10: No action needed—Go version 1.24.0 supports stdlib maps.The go.mod already specifies
go 1.24.0, which exceeds the minimum requirement of Go 1.21 for the stdlibmapspackage. The import is compatible.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
fast.go (1)
185-226: Fix potential panic at end of Fast.LookupLoop can complete without early return; panicking is unsafe. Return the best LPM found instead.
Apply:
@@ func (f *Fast[V]) Lookup(ip netip.Addr) (val V, ok bool) { - panic("unreachable") + // If traversal completes without an earlier return, + // fall back to the best LPM captured during descent. + return val, okinternal/nodes/fast.go (1)
339-356: Bug: storing &kidAny (address of range variable) risks aliasing all childrenTaking the address of the range variable kidAny can make every slot point to the same memory, leading to corrupted children after the loop. Use a fresh heap-allocated any for each entry, same as InsertChild.
Apply this diff:
- case *FastNode[V]: - // link with new *any pointer - c.Children.Items[addr] = &kidAny + case *FastNode[V]: + // link with fresh *any to avoid aliasing the range variable + p := new(any) + *p = kidAny + c.Children.Items[addr] = p - case *LeafNode[V]: - leafAny := any(kid.CloneLeaf(cloneFn)) - c.Children.Items[addr] = &leafAny + case *LeafNode[V]: + p := new(any) + *p = any(kid.CloneLeaf(cloneFn)) + c.Children.Items[addr] = p - case *FringeNode[V]: - fringeAny := any(kid.CloneFringe(cloneFn)) - c.Children.Items[addr] = &fringeAny + case *FringeNode[V]: + p := new(any) + *p = any(kid.CloneFringe(cloneFn)) + c.Children.Items[addr] = pinternal/nodes/commonmethods_tmpl.go (1)
1462-1500: COW violation in handleMatrixPersist: in-place overwrite of leaf/fringe.The "fringe+fringe" and "leaf+leaf same prefix" fast paths mutate
thisLeaf/thisFringe.Valuedirectly without cloning, breaking persistence if the child is shared. Only thethisNodecase properly clones before mutation. Replace the child in the parent slot as suggested.Apply this diff:
@@ - // Special case: fringe + fringe -> just overwrite value - if thisIsFringe && otherIsFringe { - thisFringe.Value = cloneFn(otherFringe.Value) - return 1 - } + // Special case: fringe + fringe -> replace child (COW-safe) + if thisIsFringe && otherIsFringe { + n.InsertChild(addr, &FringeNode[V]{Value: cloneFn(otherFringe.Value)}) + return 1 + } @@ - // Special case: leaf + leaf with same prefix -> just overwrite value - if thisIsLeaf && otherIsLeaf && thisLeaf.Prefix == otherLeaf.Prefix { - thisLeaf.Value = cloneFn(otherLeaf.Value) - return 1 - } + // Special case: leaf + leaf with same prefix -> replace child (COW-safe) + if thisIsLeaf && otherIsLeaf && thisLeaf.Prefix == otherLeaf.Prefix { + n.InsertChild(addr, &LeafNode[V]{Prefix: thisLeaf.Prefix, Value: cloneFn(otherLeaf.Value)}) + return 1 + }internal/nodes/litemethodsgenerated.go (2)
155-163: Fix COW violations in InsertPersist for Leaf/Fringe updates and clone values when pushing down.
- Mutating
kid.Valuein-place for Leaf/Fringe equal cases breaks persistent COW when the child is shared with the old table.- When creating
newNodeand pushing existing Leaf/Fringe down in a persistent op, pass throughcloneFn(when non-nil) to avoid cross-version aliasing.Apply this diff:
@@ - if kid.Prefix == pfx { - kid.Value = val - // exists - return true - } + if kid.Prefix == pfx { + // COW: replace leaf instead of mutating shared instance + newLeaf := *kid + newLeaf.Value = val + n.InsertChild(octet, &newLeaf) + return true + } @@ - newNode := new(LiteNode[V]) - newNode.Insert(kid.Prefix, kid.Value, depth+1) + newNode := new(LiteNode[V]) + v := kid.Value + if cloneFn != nil { + v = cloneFn(v) + } + newNode.Insert(kid.Prefix, v, depth+1) @@ - if IsFringe(depth, pfx) { - kid.Value = val - // exists - return true - } + if IsFringe(depth, pfx) { + // COW: replace fringe instead of mutating shared instance + newFringe := *kid + newFringe.Value = val + n.InsertChild(octet, &newFringe) + return true + } @@ - newNode := new(LiteNode[V]) - newNode.InsertPrefix(1, kid.Value) + newNode := new(LiteNode[V]) + v2 := kid.Value + if cloneFn != nil { + v2 = cloneFn(v2) + } + newNode.InsertPrefix(1, v2)Based on learnings
Also applies to: 175-181, 168-173, 187-192
1502-1507: Persistent union: clone values when rehoming existing Leaf/Fringe into a new node.In
handleMatrixPersist, pushingthisLeaf.Value/thisFringe.Valueintoncshould applycloneFn(when non‑nil) to prevent aliasing across versions.- case thisIsLeaf: - nc.Insert(thisLeaf.Prefix, thisLeaf.Value, depth+1) + case thisIsLeaf: + v := thisLeaf.Value + if cloneFn != nil { + v = cloneFn(v) + } + nc.Insert(thisLeaf.Prefix, v, depth+1) - case thisIsFringe: - nc.InsertPrefix(1, thisFringe.Value) + case thisIsFringe: + v2 := thisFringe.Value + if cloneFn != nil { + v2 = cloneFn(v2) + } + nc.InsertPrefix(1, v2)Based on learnings
internal/nodes/bartmethodsgenerated.go (2)
155-163: Fix COW violations in InsertPersist for Leaf/Fringe updates and clone values when pushing down.Same issues as LiteNode: avoid in-place
kid.Valuemutation and usecloneFn(when non‑nil) when relocating existing values during persistent insert.@@ - if kid.Prefix == pfx { - kid.Value = val - return true - } + if kid.Prefix == pfx { + newLeaf := *kid + newLeaf.Value = val + n.InsertChild(octet, &newLeaf) + return true + } @@ - newNode := new(BartNode[V]) - newNode.Insert(kid.Prefix, kid.Value, depth+1) + newNode := new(BartNode[V]) + v := kid.Value + if cloneFn != nil { + v = cloneFn(v) + } + newNode.Insert(kid.Prefix, v, depth+1) @@ - if IsFringe(depth, pfx) { - kid.Value = val - return true - } + if IsFringe(depth, pfx) { + newFringe := *kid + newFringe.Value = val + n.InsertChild(octet, &newFringe) + return true + } @@ - newNode := new(BartNode[V]) - newNode.InsertPrefix(1, kid.Value) + newNode := new(BartNode[V]) + v2 := kid.Value + if cloneFn != nil { + v2 = cloneFn(v2) + } + newNode.InsertPrefix(1, v2)Based on learnings
Also applies to: 175-181, 168-173, 187-192
1502-1507: Persistent union: clone values when rehoming existing Leaf/Fringe into a new node.Apply
cloneFnonthisLeaf.Value/thisFringe.Valuebefore inserting intonc.- case thisIsLeaf: - nc.Insert(thisLeaf.Prefix, thisLeaf.Value, depth+1) + case thisIsLeaf: + v := thisLeaf.Value + if cloneFn != nil { v = cloneFn(v) } + nc.Insert(thisLeaf.Prefix, v, depth+1) - case thisIsFringe: - nc.InsertPrefix(1, thisFringe.Value) + case thisIsFringe: + v2 := thisFringe.Value + if cloneFn != nil { v2 = cloneFn(v2) } + nc.InsertPrefix(1, v2)Based on learnings
internal/nodes/fastmethodsgenerated.go (2)
155-163: Fix COW violations in InsertPersist for Leaf/Fringe updates and clone values when pushing down.Avoid in-place
kid.Valuemutation and usecloneFn(when non‑nil) when relocating existing values during persistent insert.@@ - if kid.Prefix == pfx { - kid.Value = val - return true - } + if kid.Prefix == pfx { + newLeaf := *kid + newLeaf.Value = val + n.InsertChild(octet, &newLeaf) + return true + } @@ - newNode := new(FastNode[V]) - newNode.Insert(kid.Prefix, kid.Value, depth+1) + newNode := new(FastNode[V]) + v := kid.Value + if cloneFn != nil { v = cloneFn(v) } + newNode.Insert(kid.Prefix, v, depth+1) @@ - if IsFringe(depth, pfx) { - kid.Value = val - return true - } + if IsFringe(depth, pfx) { + newFringe := *kid + newFringe.Value = val + n.InsertChild(octet, &newFringe) + return true + } @@ - newNode := new(FastNode[V]) - newNode.InsertPrefix(1, kid.Value) + newNode := new(FastNode[V]) + v2 := kid.Value + if cloneFn != nil { v2 = cloneFn(v2) } + newNode.InsertPrefix(1, v2)Based on learnings
Also applies to: 175-181, 168-173, 187-192
1502-1507: Persistent union: clone values when rehoming existing Leaf/Fringe into a new node.Apply
cloneFnonthisLeaf.Value/thisFringe.Valuebefore inserting intonc.- case thisIsLeaf: - nc.Insert(thisLeaf.Prefix, thisLeaf.Value, depth+1) + case thisIsLeaf: + v := thisLeaf.Value + if cloneFn != nil { v = cloneFn(v) } + nc.Insert(thisLeaf.Prefix, v, depth+1) - case thisIsFringe: - nc.InsertPrefix(1, thisFringe.Value) + case thisIsFringe: + v2 := thisFringe.Value + if cloneFn != nil { v2 = cloneFn(v2) } + nc.InsertPrefix(1, v2)Based on learnings
🧹 Nitpick comments (25)
zz-example_table_concurrent_test.go (3)
14-16: Use method-based wording for cloning (avoid “Cloner interface”).Prefer describing the required method explicitly to align with structural typing.
Apply:
-// We use *testVal as the generic payload type V, which is a pointer type, -// so it must implement Cloner[V]. +// We use *testVal as the generic payload type V (a pointer type). +// If V is a pointer or contains pointers, implement: +// func (v *testVal) Clone() *testVal +// The bart package detects this via structural typing at runtime.
20-22: Tighten Clone comment to method contract.Clarify that Clone enables deep copies detected structurally, not via a named interface.
-// Clone ensures deep copying for use with ...Persist implementing the -// Cloner interface. +// Clone enables deep copying for ...Persist operations. +// Detected via structural typing (presence of a matching Clone method).
42-44: Rephrase to “implement Clone()” rather than “implement the Cloner interface”.Keeps public docs consistent with structural typing guidance.
-// If the payload V either contains a pointer or is a pointer, -// it must implement the Cloner interface. +// If the payload V either contains pointers or is a pointer, +// implement a Clone method (structural typing is used).zz-example_fast_concurrent_test.go (1)
26-28: Align wording with structural typing (no named interface).Prefer stating the required Clone method instead of “Cloner interface”.
-// If the payload V either contains or is a pointer, -// it must implement the Cloner interface. +// If the payload V either contains pointers or is a pointer, +// implement a Clone method so persistent ops can deep-copy values.commonmethods_tmpl.go (5)
122-124: Doc consistency: prefer method-based wording over “Cloner interface”.Elsewhere in this file you already use “implement the Clone method”. Make this block consistent.
-// If the payload type V contains pointers or needs deep copying, -// it must implement the value.Cloner interface to support correct cloning. +// If the payload type V contains pointers or needs deep copying, +// implement: +// func (v V) Clone() V +// The bart package detects this via structural typing and deep‑copies values during persistent ops.
142-145: Clarify CloneFnFactory behavior without naming an interface.-// Create a cloning function for deep copying values; -// returns nil if V does not implement the value.Cloner interface. +// Create a cloning function for deep copying values; +// returns nil if V does not provide a Clone() V method.
245-248: Repeat the method-based phrasing here too.-// Create a cloning function for deep copying values; -// returns nil if V does not implement the value.Cloner interface. +// Create a cloning function for deep copying values; +// returns nil if V does not provide a Clone() V method.
443-447: Same terminology fix in Union().-// Create a cloning function for deep copying values; -// returns nil if V does not implement the value.Cloner interface. +// Create a cloning function for deep copying values; +// returns nil if V lacks a Clone() V method.
464-467: Same terminology fix in UnionPersist().-// Create a cloning function for deep copying values; -// returns nil if V does not implement the value.Cloner interface. +// Create a cloning function for deep copying values; +// returns nil if V lacks a Clone() V method.lite.go (1)
156-163: Public Modify wrapper looks good; consider noting canonicalization.The behavior matches other APIs; add a short note that pfx is Masked internally for consistency.
-// Modify applies an insert, update, or delete operation for the given -// prefix. +// Modify applies an insert, update, or delete for the given prefix. +// The prefix is canonicalized (Masked) internally before the operation.bart.go (1)
77-118: Clarify callback invocation guarantee in Modify docsState explicitly that cb is invoked at most once per call for the given prefix.
Apply this minimal doc tweak:
@@ // del == true deletes the entry if it exists (otherwise no-op). // +// The callback is invoked at most once per call. // // The operation is determined by the callback function, which is called with:fast.go (2)
67-70: Defer ZST check until after prefix validationAvoid panicking on invalid prefixes by moving the ZST check after IsValid. Behavior is unchanged for valid inputs.
Apply:
@@ func (t *Fast[V]) Insert(pfx netip.Prefix, val V) { - t.once.Do(value.PanicOnZST[V]) - t.insert(pfx, val) + if !pfx.IsValid() { + return + } + t.once.Do(value.PanicOnZST[V]) + t.insert(pfx, val) @@ func (t *Fast[V]) InsertPersist(pfx netip.Prefix, val V) *Fast[V] { - t.once.Do(value.PanicOnZST[V]) - return t.insertPersist(pfx, val) + if !pfx.IsValid() { + return t + } + t.once.Do(value.PanicOnZST[V]) + return t.insertPersist(pfx, val) @@ func (t *Fast[V]) Modify(pfx netip.Prefix, cb func(_ V, ok bool) (_ V, del bool)) { - t.once.Do(value.PanicOnZST[V]) - if !pfx.IsValid() { + if !pfx.IsValid() { return } + t.once.Do(value.PanicOnZST[V])Also applies to: 83-86, 114-119
61-71: Minor: unify receiver naming (t vs f) within fast.goConsistently use a single receiver name (e.g., f) across Fast methods for readability.
Also applies to: 72-86, 88-129
fastmethodsgenerated.go (3)
61-66: Doc wording: avoid exposing internal/value types in commentsPrefer “types that implement Clone() V” over referencing value.Cloner interface.
Apply:
@@ -// If the payload type V contains pointers or needs deep copying, -// it must implement the value.Cloner interface to support correct cloning. +// If the payload type V contains pointers or needs deep copying, +// it must implement a Clone() V method to support correct cloning.
184-188: Doc wording consistency for DeletePersistMatch InsertPersist phrasing.
Apply:
@@ -// If the payload type V contains pointers or requires deep copying, -// it must implement the Clone method for correct cloning. +// If the payload type V contains pointers or requires deep copying, +// it must implement a Clone() V method for correct cloning.
382-386: Doc wording consistency in UnionUse method-based phrasing here too.
Apply:
@@ -// This duplicate is shallow-copied by default, but if the value type V implements the -// Clone method, the value is deeply cloned before insertion. See also Fast.Clone. +// This duplicate is shallow-copied by default, but if V implements Clone() V, +// the value is deeply cloned before insertion. See also Fast.Clone.internal/nodes/fast.go (1)
372-377: Consistency/safety: avoid &local var; allocate new(any) for child assignmentWhile &nodeAny escapes to heap and works, prefer the same new(any) pattern as elsewhere to keep semantics uniform and avoid future refactor hazards.
- case *FastNode[V]: - nodeAny := any(kid.CloneRec(cloneFn)) - c.Children.Items[addr] = &nodeAny + case *FastNode[V]: + p := new(any) + *p = any(kid.CloneRec(cloneFn)) + c.Children.Items[addr] = pPlease re-run fast node deep-clone tests after this change to ensure identical behavior.
internal/value/value_test.go (1)
112-166: Add typed‑nil equality cases to harden Equal() behaviorTyped nils inside interfaces are a classic edge case. Extend TestEqual with pointer/slice/map typed‑nil scenarios.
@@ func TestEqual(t *testing.T) { t.Parallel() + t.Run("typed_nil_interfaces", func(t *testing.T) { + t.Parallel() + // pointer typed-nil + var p1 *int = nil + var p2 *int = nil + if !Equal[any](p1, p2) { + t.Error("Equal should treat two typed-nil pointers as equal") + } + // slice typed-nil + var s1 []int = nil + var s2 []int = nil + if !Equal[any](s1, s2) { + t.Error("Equal should treat two typed-nil slices as equal") + } + // map typed-nil + var m1 map[string]int = nil + var m2 map[string]int = nil + if !Equal[any](m1, m2) { + t.Error("Equal should treat two typed-nil maps as equal") + } + // interface holding typed-nil vs untyped nil + var ai any = (*int)(nil) + var bi any = nil + if Equal(ai, bi) { + t.Error("Equal should not treat typed-nil inside interface equal to nil interface") + } + }) @@ }I can add analogous cases for Clone/Copy if desired.
litemethodsgenerated.go (1)
81-84: Docs: prefer method‑shape over internal interface namePublic docs mention value.Cloner. Since Cloner is internal, prefer “implement Clone() V” phrasing consistently (some places already do). This avoids leaking internal package details to users.
Also applies to: 184-187, 382-385, 403-406
internal/nodes/bart.go (1)
121-134: Verify AsSlice()/Items index alignmentAllChildren indexes child via Items[i] while iterating addrs from AsSlice(). This assumes sparse.Array256 guarantees Items[i] corresponds to addrs[i]. If that coupling isn’t explicit, switch to MustGetChild(addr) or Children.Get(addr) to avoid mismatch.
Example change:
- addrs := n.Children.AsSlice(&buf) - for i, addr := range addrs { - child := n.Children.Items[i] + for _, addr := range n.Children.AsSlice(&buf) { + child, _ := n.Children.Get(addr)bartmethodsgenerated.go (1)
81-84: Docs: use method signature, not internal interface nameAs with liteTable, prefer “implement Clone() V” verbiage consistently across public docs to avoid exposing internal/value.Cloner.
Also applies to: 184-187, 382-385, 403-406
internal/nodes/litetestsgenerated_test.go (2)
642-646: Intentional skip for Lite “non‑ZST” value assertions.This test will always skip because Lite doesn’t carry payloads. If you keep the symmetry, consider renaming to make the skip explicit (e.g., TestDump_NonZST_LiteNode_Skip) to avoid confusion in reports. Based on learnings.
If you prefer, I can push a tiny rename commit to make the skip intent clearer.
709-713: Intentional skip for Lite “non‑ZST” FprintRec.Same rationale as above; optional rename could reduce confusion in CI summaries. Based on learnings.
internal/nodes/nodebasics.go (1)
1-1: Add tiny tests to raise coverage on new nil‑receiver branches.Consider unit tests for CloneLeaf(nil) and CloneFringe(nil) returning nil, plus non‑nil with nil/nonnull cloneFn. This should close part of the reported uncovered lines in nodebasics.
I can draft minimal tests targeting these branches if helpful.
internal/value/value.go (1)
86-98: Equal prefers Equaler, falls back to reflect.DeepEqual.Solid default. Consider documenting nil interface nuances (typed‑nil pointers), though CloneVal already handles nil receivers. Optional only.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
bart.go(1 hunks)bartmethodsgenerated.go(10 hunks)common.go(1 hunks)commonmethods_tmpl.go(10 hunks)fast.go(3 hunks)fastmethodsgenerated.go(10 hunks)internal/nodes/bart.go(3 hunks)internal/nodes/bartmethodsgenerated.go(18 hunks)internal/nodes/barttestsgenerated_test.go(8 hunks)internal/nodes/commonmethods_tmpl.go(18 hunks)internal/nodes/commontests_tmpl.go(9 hunks)internal/nodes/fast.go(3 hunks)internal/nodes/fastmethodsgenerated.go(18 hunks)internal/nodes/fasttestsgenerated_test.go(8 hunks)internal/nodes/lite.go(3 hunks)internal/nodes/litemethodsgenerated.go(18 hunks)internal/nodes/litetestsgenerated_test.go(8 hunks)internal/nodes/nodebasics.go(3 hunks)internal/nodes/nodebasics_test.go(3 hunks)internal/value/value.go(1 hunks)internal/value/value_test.go(1 hunks)lite.go(2 hunks)litemethodsgenerated.go(10 hunks)zz-example_custom_value_test.go(1 hunks)zz-example_fast_concurrent_test.go(1 hunks)zz-example_table_concurrent_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (29)
📚 Learning: 2025-09-12T20:44:35.654Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-12T20:44:35.654Z
Learning: In this codebase, Cloner[V] is applied during structural cloning of persistent tables (cloneFlat with cloneFn) and not at the moment a brand-new value is inserted. InsertPersist/ModifyPersist store new values as-is; cloning occurs when existing values are carried into a new persistent table. Modify/ModifyPersist return oldVal on updates, newVal on inserts, with deleted indicating deletion.
Applied to files:
fastmethodsgenerated.gozz-example_table_concurrent_test.golitemethodsgenerated.gocommonmethods_tmpl.gointernal/nodes/commonmethods_tmpl.gointernal/nodes/fastmethodsgenerated.gointernal/nodes/litemethodsgenerated.go
📚 Learning: 2025-09-23T14:26:38.307Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-23T14:26:38.307Z
Learning: In gaissmai/bart cloner.go, cloneFlat for bartNode[V], fastNode[V], and liteNode[V] accepts a cloneFn cloneFunc[V]; passing nil is intentional and means “clone structure only, do not clone values”. Only when cloneFn != nil are the node’s values deep-cloned. Do not flag nil cloneFn usage in cloneFlat call sites.
Applied to files:
fastmethodsgenerated.gocommon.gointernal/nodes/fast.gointernal/nodes/nodebasics_test.golitemethodsgenerated.gobartmethodsgenerated.gointernal/nodes/bart.gocommonmethods_tmpl.gointernal/nodes/nodebasics.gointernal/nodes/lite.gointernal/nodes/bartmethodsgenerated.go
📚 Learning: 2025-09-28T17:25:40.688Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-28T17:25:40.688Z
Learning: For ModifyPersist in Table/Fast/Lite, prefer a simple preflight Get() to read (old, exists), call cb once, then dispatch to no-op, InsertPersist (upsert), or DeletePersist. Clone old for cb only if cloner != nil; this preserves prior semantics without structural cloning on no-op.
Applied to files:
fastmethodsgenerated.gobart.gofast.golitemethodsgenerated.gobartmethodsgenerated.golite.gocommonmethods_tmpl.go
📚 Learning: 2025-09-09T14:41:43.277Z
Learnt from: gaissmai
PR: gaissmai/bart#284
File: cloner.go:28-30
Timestamp: 2025-09-09T14:41:43.277Z
Learning: In Go generics, you cannot use `val == nil` directly on an unconstrained type parameter V because nil comparison requires the type to be constrained to nullable types. The correct approach for nil checking with generics is to use type assertion with comma-ok pattern: `cloner, ok := any(val).(Cloner[V])` followed by checking both `ok` and whether `cloner == nil`.
Applied to files:
fastmethodsgenerated.gozz-example_table_concurrent_test.gointernal/nodes/nodebasics_test.golitemethodsgenerated.gobartmethodsgenerated.gocommonmethods_tmpl.go
📚 Learning: 2025-09-13T07:39:52.762Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-13T07:39:52.762Z
Learning: For bart routing table test files, use routeEntry struct with nextHop netip.Addr, exitIF string, and attributes map[string]int fields, with *routeEntry as the Fat[V] payload type in tests, instead of generic configuration objects. Implement Clone() method for deep cloning of the attributes map.
Applied to files:
fastmethodsgenerated.gozz-example_custom_value_test.gocommon.golitemethodsgenerated.gocommonmethods_tmpl.go
📚 Learning: 2025-09-13T07:30:12.421Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-13T07:30:12.421Z
Learning: For bart routing table documentation and examples, use routeEntry struct with nextHop netip.Addr, exitIF string, and attributes map[string]int fields, with *routeEntry as the Fat[V] payload type, instead of generic configuration objects.
Applied to files:
zz-example_custom_value_test.go
📚 Learning: 2025-09-28T14:26:04.273Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-28T14:26:04.273Z
Learning: In fastNode.cloneFlat, children of type leafNode[V] and fringeNode[V] must be cloned (cloneFlat), while inner fastNode children may remain shared; prefixes are rebuilt via allot, and immediate containers are duplicated to avoid aliasing/races.
Applied to files:
internal/nodes/fast.go
📚 Learning: 2025-09-12T06:23:05.913Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-12T06:23:05.913Z
Learning: The leafNode[V] struct in this codebase has two fields: `prefix netip.Prefix` and `value V`. The prefix field is of type netip.Prefix, not []byte. When generating test code for leafNode, use netip.MustParsePrefix() or similar functions to create valid netip.Prefix values.
Applied to files:
internal/nodes/commontests_tmpl.gointernal/nodes/bart.gointernal/nodes/lite.gointernal/nodes/commonmethods_tmpl.gointernal/nodes/fastmethodsgenerated.gointernal/nodes/bartmethodsgenerated.gointernal/nodes/litemethodsgenerated.go
📚 Learning: 2025-10-05T07:11:38.369Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T07:11:38.369Z
Learning: LiteNode in the bart codebase does not use the allot mechanism like FastNode does. LiteNode.Contains() checks for ancestor prefixes in the complete binary tree using LookupTbl, not descendants. When testing LiteNode, ensure tests reflect ancestor relationships (idx, idx>>1, idx>>2, ...) rather than descendant propagation (idx<<1, (idx<<1)+1, ...).
Applied to files:
internal/nodes/commontests_tmpl.gointernal/nodes/barttestsgenerated_test.gointernal/nodes/fasttestsgenerated_test.gointernal/nodes/lite.gointernal/nodes/litetestsgenerated_test.go
📚 Learning: 2025-10-05T07:12:24.637Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T07:12:24.637Z
Learning: LiteNode in the bart codebase does not use the allot mechanism like FastNode does. LiteNode.Contains() checks for ancestor prefixes in the complete binary tree using LookupTbl[idx], not descendants. When testing LiteNode, ensure tests reflect ancestor relationships (idx, idx>>1, idx>>2, ...) rather than descendant propagation. The allot table mentioned by the maintainer (32→64,65,128,...) applies to FastNode's descendant propagation, not LiteNode's ancestor checking.
Applied to files:
internal/nodes/commontests_tmpl.gointernal/nodes/barttestsgenerated_test.gointernal/nodes/fasttestsgenerated_test.gointernal/nodes/lite.gointernal/nodes/litetestsgenerated_test.go
📚 Learning: 2025-10-05T19:56:18.121Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T19:56:18.121Z
Learning: In internal/nodes LiteNode does not store values; tests for Modify/Insert/Get must assert existence (ok) rather than value equality. Prefer Get(pfx) for verification and avoid asserting returned values.
Applied to files:
internal/nodes/commontests_tmpl.gointernal/nodes/lite.gointernal/nodes/litetestsgenerated_test.go
📚 Learning: 2025-10-19T06:22:03.283Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-19T06:22:03.283Z
Learning: Tests that verify modified payload values should skip liteTable because liteTable uses struct{} as its payload type and has no real payload to verify modifications on.
Applied to files:
internal/nodes/commontests_tmpl.golite.gointernal/nodes/barttestsgenerated_test.gointernal/nodes/litetestsgenerated_test.go
📚 Learning: 2025-10-05T21:46:51.386Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:46:51.386Z
Learning: In internal/nodes, FastNode and LiteNode expose DumpString(octets []uint8, depth int, is4 bool, printVals bool) string with identical traversal/error semantics as BartNode; tests must construct multi-level paths via InsertChild with node-typed children to avoid Fringe/Leaf along the traversal path, and assert the two error branches (kid not set, kid wrong type).
Applied to files:
internal/nodes/commontests_tmpl.gointernal/nodes/barttestsgenerated_test.gointernal/nodes/fasttestsgenerated_test.gointernal/nodes/commonmethods_tmpl.gointernal/nodes/fastmethodsgenerated.gointernal/nodes/litetestsgenerated_test.gointernal/nodes/bartmethodsgenerated.gointernal/nodes/litemethodsgenerated.go
📚 Learning: 2025-10-28T20:34:10.928Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-28T20:34:10.928Z
Learning: In the bart codebase, zero-sized types (ZSTs) are omitted from dump and print output not for performance reasons, but to improve readability. ZST values carry no information and create unnecessary line noise that makes debug output harder to read.
Applied to files:
internal/nodes/commontests_tmpl.gointernal/nodes/barttestsgenerated_test.gointernal/nodes/fasttestsgenerated_test.gointernal/nodes/litetestsgenerated_test.go
📚 Learning: 2025-10-05T21:26:24.231Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:26:24.231Z
Learning: BartNode DumpString signature in internal/nodes: DumpString(octets []uint8, depth int, is4 bool, printVals bool) string. It traverses down the child path (expecting *BartNode at each step) before dumping the subtree with Dump. Tests must pass octets (first N octets) and a matching depth to target deeper nodes.
Applied to files:
internal/nodes/commontests_tmpl.gointernal/nodes/barttestsgenerated_test.gointernal/nodes/fasttestsgenerated_test.gointernal/nodes/commonmethods_tmpl.gointernal/nodes/fastmethodsgenerated.gointernal/nodes/litetestsgenerated_test.gointernal/nodes/bartmethodsgenerated.gointernal/nodes/litemethodsgenerated.go
📚 Learning: 2025-10-05T21:27:32.948Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:27:32.948Z
Learning: BartNode DumpString signature in internal/nodes: func (n *BartNode[V]) DumpString(octets []uint8, depth int, is4 bool, printVals bool) string. It traverses down the child chain using GetChild(path[i]) and requires BartNode children along the octet/depth path; otherwise it returns an "ERROR: kid ..." string. Tests must ensure traversal paths are BartNode nodes (e.g., by inserting BartNode children via InsertChild) instead of Fringe/Leaf children.
Applied to files:
internal/nodes/bart.gointernal/nodes/barttestsgenerated_test.gointernal/nodes/commonmethods_tmpl.gointernal/nodes/fastmethodsgenerated.gointernal/nodes/bartmethodsgenerated.gointernal/nodes/litemethodsgenerated.go
📚 Learning: 2025-09-21T10:25:06.668Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-21T10:25:06.668Z
Learning: BART implements three node types with different memory/performance tradeoffs: bartNode[V] (112 bytes + data, balanced), liteNode[V] (96 bytes + children, no values), fastNode[V] (4,168 bytes fixed, 40% faster). All achieve O(1) per-level lookup but bartNode/liteNode use rank-based indirection while fastNode uses direct array access.
Applied to files:
internal/nodes/bart.go
📚 Learning: 2025-10-07T17:43:21.478Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-07T17:43:21.478Z
Learning: liteNode in gaissmai/bart stores no values in LiteLeafNode (32 bytes: just netip.Prefix with 24B Addr + 1B uint8 + 7B padding) and LiteFringeNode (0 bytes payload, prefix implicit). This makes liteNode more memory-efficient than bartNode/fastNode which store values in their LeafNode (40 bytes: 8B value + 32B prefix) and FringeNode (8 bytes). When calculating bytes per prefix, divide total bytes by (prefixes + leaves + fringes).
Applied to files:
internal/nodes/bart.gointernal/nodes/lite.go
📚 Learning: 2025-10-05T21:31:48.381Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-05T21:31:48.381Z
Learning: In internal/nodes BartNode, inserting prefixes like /8, /16, /24 via Insert creates Fringe/Leaf, never normal BartNode nodes. For DumpString tests that must traverse deeper via octets/depth, construct the path with InsertChild to attach BartNode children; populate those nodes via InsertPrefix so the dump has content.
Applied to files:
internal/nodes/bart.gointernal/nodes/barttestsgenerated_test.gointernal/nodes/bartmethodsgenerated.go
📚 Learning: 2025-09-20T22:32:23.233Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-20T22:32:23.233Z
Learning: All three BART node types (bartNode, liteNode, fastNode) achieve O(1) lookup performance through precomputed lookup tables (lmp.LookupTbl[idx]) and BitSet256 intersection operations, not O(log n) as might be expected from "sparse" data structures.
Applied to files:
internal/nodes/bart.go
📚 Learning: 2025-09-21T17:48:39.206Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-21T17:48:39.206Z
Learning: In the gaissmai/bart codebase, the Lite type embeds liteTable[struct{}] directly, making methods like Contains, Delete, Get, and Modify automatically available on Lite instances through Go's method promotion without needing any adapter methods.
Applied to files:
lite.go
📚 Learning: 2025-09-21T17:48:39.206Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-21T17:48:39.206Z
Learning: In the gaissmai/bart codebase, the Lite type embeds liteTable[struct{}] directly using Go's embedding syntax. All methods from liteTable[struct{}] are automatically promoted to Lite without needing adapter methods due to Go's embedding mechanism.
Applied to files:
lite.go
📚 Learning: 2025-09-17T13:38:33.273Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-17T13:38:33.273Z
Learning: The Lite type in the gaissmai/bart codebase is not generic and should be used as &Lite{} instead of &Lite[T]{}. The Lite.Insert() method only takes a prefix parameter, not a value parameter, as it only stores prefixes.
Applied to files:
lite.gointernal/nodes/lite.go
📚 Learning: 2025-09-21T17:48:39.206Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-21T17:48:39.206Z
Learning: In the gaissmai/bart codebase, the Lite type embeds liteTable[struct{}] directly, so all methods from liteTable[struct{}] are automatically promoted to Lite without needing adapter methods. Go's embedding provides automatic method promotion.
Applied to files:
lite.go
📚 Learning: 2025-09-29T18:27:16.452Z
Learnt from: gaissmai
PR: gaissmai/bart#324
File: nodecommon.go:28-30
Timestamp: 2025-09-29T18:27:16.452Z
Learning: In gaissmai/bart codebase using Go 1.24+, the stridePath type defined as [maxTreeDepth]uint8 works correctly with netip calls like netip.AddrFrom16(path) and netip.AddrFrom4([4]byte(path[:4])) without needing type aliases. Go 1.24+ supports these type conversions natively.
Applied to files:
internal/nodes/nodebasics.go
📚 Learning: 2025-09-30T08:30:12.716Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-30T08:30:12.716Z
Learning: In the bart tests, prefer the existing helpers mpp (MustParsePrefix) and mpa (MustParseAddr); do not re-declare local equivalents in new *_test.go files.
Applied to files:
internal/nodes/barttestsgenerated_test.go
📚 Learning: 2025-09-19T07:28:23.239Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-19T07:28:23.239Z
Learning: In the bart repository nodeiface_test.go file, the constant declarations like `const c = uint8(100)` at line 984 are already properly scoped within their respective test functions and should not be moved.
Applied to files:
internal/nodes/barttestsgenerated_test.go
📚 Learning: 2025-09-29T07:02:34.949Z
Learnt from: gaissmai
PR: gaissmai/bart#323
File: litetablemethods_gen.go:66-103
Timestamp: 2025-09-29T07:02:34.949Z
Learning: In litebasics_gen.go's insertPersist method, there is a copy-on-write bug in the leafNode case where kid.value = val directly mutates the shared leafNode without cloning it first, violating COW semantics. The liteNode case correctly clones with kid.cloneFlat(cloneFn) before mutation.
Applied to files:
internal/nodes/lite.gointernal/nodes/litemethodsgenerated.go
📚 Learning: 2025-09-23T11:56:58.877Z
Learnt from: gaissmai
PR: gaissmai/bart#317
File: nodeunion_tmpl.go:426-430
Timestamp: 2025-09-23T11:56:58.877Z
Learning: In nodeunion_tmpl.go's unionRecPersist method, fringe nodes (thisKid) are already properly cloned before reaching the fringe/fringe case, so direct mutation of thisKid.value is safe and does not cause persistent aliasing issues.
Applied to files:
internal/nodes/commonmethods_tmpl.gointernal/nodes/fastmethodsgenerated.gointernal/nodes/bartmethodsgenerated.gointernal/nodes/litemethodsgenerated.go
🔇 Additional comments (38)
zz-example_custom_value_test.go (1)
10-35: Nice, clear custom value example.Equality and deep cloning are correctly showcased; maps.Equal/Clone handle nil safely. Example intent is precise.
commonmethods_tmpl.go (3)
219-221: LGTM: method-based wording here is correct.This block already uses “implement the Clone method”; no change needed.
494-511: Equality docs are clear and accurate.Good explanation of Equal(other V) structural typing and avoiding reflect.DeepEqual.
522-537: Clone docs read well.Method-based contract and example are consistent with the rest.
lite.go (3)
167-175: ModifyPersist wrapper LGTM.Thin adapter is correct and mirrors Modify.
522-563: liteTable.Modify is correct and efficient.
- Validity check, masking, per‑version routing: good.
- Delegates to node.Modify and updates size via delta: good.
392-397: Structural-equality note for Lite is clear.Accurately sets expectations for users.
internal/nodes/nodebasics_test.go (1)
11-13: Tests align with new internal/value APIsImport and assertions updated correctly; typed-nil handling in cloneVal is sound.
Also applies to: 28-35, 41-47
common.go (1)
26-38: Doc additions are clear and helpfulClone/Equal guidance via structural typing is well explained.
internal/nodes/bart.go (1)
191-233: Cloning logic reads well; value cloning gated by CloneFnCloneFlat applies cloneFn to prefix values and clones leaf/fringe; CloneRec deep‑clones BartNode descendants. LGTM.
Also applies to: 249-264
internal/nodes/commontests_tmpl.go (1)
639-663: ZST/non‑ZST dump and FprintRec tests look solidAssertions align with intended behavior (omit values for ZST; include for non‑ZST). LiteNode skip guards are correct.
Also applies to: 666-699, 702-730, 733-768
bartmethodsgenerated.go (1)
217-241: ModifyPersist preserves prior semantics; cloning only for cb when neededPreflight Get + conditional cloneFn is the right trade‑off. LGTM.
Consider adding a small test ensuring cb observes a deep clone when V clones, and shared state when it doesn’t (for Table and Lite variants).
internal/nodes/fasttestsgenerated_test.go (1)
636-670: Tests: good coverage on dump/FprintRec ZST vs non‑ZSTGuards and assertions look correct and precise. Nice.
Also applies to: 672-701, 703-739
internal/nodes/barttestsgenerated_test.go (1)
636-670: Tests: BartNode ZST/non‑ZST printing validated; Lite skip fixedAll good here.
Also applies to: 672-701, 703-739
internal/nodes/litetestsgenerated_test.go (2)
609-634: ZST dump assertions look correct for Lite.Checks align with dump’s printValues gate and Lite’s “no payload” design. Good coverage for the “no values(#)” section. Based on learnings.
697-701: ZST FprintRec validation is precise.Matching the prefix directly followed by “(” avoids false positives and mirrors the intended omission for ZSTs. Nice. Based on learnings.
internal/nodes/nodebasics.go (4)
338-347: Nil‑receiver safe CloneLeaf.Good addition: avoids panics and preserves cloneFn semantics when provided.
352-361: Nil‑receiver safe CloneFringe.Consistent with CloneLeaf; looks correct.
836-877: dump: value omission toggled via IsZST — matches tests.The printValues guard is clean and centralizes the ZST decision. No changes needed.
1109-1139: FprintRec: ZST omission logic is consistent.Using the same printValues flag keeps behaviors aligned between dump and FprintRec. LGTM.
internal/nodes/commonmethods_tmpl.go (3)
1294-1323: Defaulting cloneFn to value.CopyVal is sensible.Good fallback ensuring predictable semantics when callers don’t provide a cloner.
832-958: dump/print values gated by IsZST — consistent with tests.The printValues switch correctly omits “values(#)” and value payloads for ZSTs; leaf/fringe branches follow the same rule. Matches the new tests. Based on learnings.
149-234: Using learnings from the repository author (gaissmai):The review comment flagging COW violations in InsertPersist is incorrect. The learnings explicitly address this exact concern:
- Learning (2025-09-29): "Do not flag direct mutations on leafNode/fringeNode as COW violations when they occur within persist operations, as the nodes are already properly isolated."
- Learning (2025-09-28): "in persist purge/compress, the stack nodes have already been cloneFlat during the descent, so re-inserting or mutating leaf/fringe under those cloned parents is persistence-safe."
The persistence model is:
- Table-level clones the root node before calling node-level InsertPersist
- Node-level InsertPersist clones internal (_NODE_TYPE) nodes during descent
- When reaching a matching LeafNode or FringeNode under a cloned parent, mutating its value is safe—the node is isolated within the cloned subtree
The code pattern of
kid.Value = valwithout local CloneFlat in the LeafNode/FringeNode cases is intentional and safe, not a bug.Likely an incorrect or invalid review comment.
internal/nodes/lite.go (3)
15-37: Clear documentation: LiteNode has phantom V and no payload.The comments and struct layout make the intent explicit. This aligns with the project’s LiteNode design. Based on learnings.
180-201: CloneFlat ignores cloneFn — correct for value‑less Lite.Shallow copying bitsets and sparse array is sufficient; no payload to deep‑clone.
216-232: CloneRec deep‑clones only sub‑LiteNodes.Reasonable trade‑off: leaves/fringes are value‑less under Lite and can be shared. LGTM.
internal/value/value.go (3)
41-58: IsZST without unsafe is acceptable.Heap‑escape + pointer equality is a pragmatic approach given the “no unsafe” constraint previously discussed. LGTM.
111-121: CloneFnFactory returning nil when not a Cloner keeps call sites simple.Nice pattern to signal “no deep clone needed” without extra checks.
127-135: CloneVal correctly guards typed‑nil receivers.The (c == nil) check avoids panics when val is a nil pointer implementing Cloner. Good.
internal/nodes/litemethodsgenerated.go (3)
1458-1466: Verify in-place overwrite in persist path doesn’t violate COW.
leaf+leaf(same prefix)andfringe+fringeoverwrite mutatethisLeaf/thisFringein place. Safe only if that child is already detached (cloned) from the old version. Please confirm the invariant or replace the child with a new Leaf/Fringe as in the InsertPersist fix.Based on learnings
Also applies to: 1457-1460
714-717: Good: value-aware equality.Using
value.EqualhandlesEqualer[V]and typed‑nil cases.Also applies to: 748-751, 760-763
807-808: Good: ZST-gated printing and dump refactor.
printValues := !value.IsZST[V]()and routing throughdumpreduces noise for ZSTs and keeps behavior consistent across Dump/DumpRec/DumpString/FprintRec.Also applies to: 831-842, 882-887, 902-907, 787-789, 958-959, 1083-1105
internal/nodes/bartmethodsgenerated.go (3)
1458-1466: Verify in-place overwrite in persist path doesn’t violate COW.
leaf+leaf(same prefix)andfringe+fringeoverwrite mutate the existing child. Confirm the child is guaranteed cloned/detached beforehand; otherwise replace the child as in InsertPersist.Based on learnings
Also applies to: 1457-1460
714-717: Good: value-aware equality.
value.Equaluse is correct.Also applies to: 748-751, 760-763
807-808: Good: ZST-gated printing and dump refactor.Consistent value suppression for ZST and internal
dumpusage.Also applies to: 831-842, 882-887, 902-907, 787-789, 958-959, 1083-1105
internal/nodes/fastmethodsgenerated.go (3)
1458-1466: Verify in-place overwrite in persist path doesn’t violate COW.
leaf+leaf(same prefix)andfringe+fringeoverwrite mutate the existing child. Confirm it’s already cloned/detached; otherwise replace the child to maintain immutability.Based on learnings
Also applies to: 1457-1460
714-717: Good: value-aware equality.
value.Equalusage looks correct.Also applies to: 748-751, 760-763
807-808: Good: ZST-gated printing and dump refactor.Consistent and clean output behavior for ZSTs across Dump/FprintRec.
Also applies to: 831-842, 882-887, 902-907, 787-789, 958-959, 1083-1105
@coderabbitai yes, please do |
|
Tip For best results, initiate chat on the files or code changes. @gaissmai: Gerne! Hier sind die typed nil Tests für Clone/Copy: func TestCloneVal_TypedNil(t *testing.T) {
t.Parallel()
t.Run("pointer_typed_nil_without_Cloner", func(t *testing.T) {
var p *int
cloned := CloneVal(p)
if cloned != nil {
t.Error("CloneVal should preserve typed nil pointer")
}
})
t.Run("pointer_typed_nil_with_Cloner", func(t *testing.T) {
type clonablePtr struct {
Val *int
}
// Note: we'd need to implement Clone() for this to work
// For now, without Cloner, typed nil is passed through
var cp clonablePtr
cloned := CloneVal(cp)
if cloned.Val != nil {
t.Error("CloneVal should preserve typed nil in struct fields")
}
})
t.Run("slice_typed_nil", func(t *testing.T) {
var s []int
cloned := CloneVal(s)
if cloned != nil {
t.Error("CloneVal should preserve typed nil slice")
}
})
t.Run("map_typed_nil", func(t *testing.T) {
var m map[string]int
cloned := CloneVal(m)
if cloned != nil {
t.Error("CloneVal should preserve typed nil map")
}
})
t.Run("interface_with_typed_nil", func(t *testing.T) {
var i interface{} = (*int)(nil)
cloned := CloneVal(i)
if cloned != i {
t.Error("CloneVal should preserve interface with typed nil")
}
// Verify it's still typed nil, not just nil
if cloned == nil {
t.Error("CloneVal should preserve typed nil, not convert to untyped nil")
}
})
t.Run("struct_with_all_nil_fields", func(t *testing.T) {
type testStruct struct {
Ptr *int
Slice []string
Map map[int]string
}
var s testStruct
cloned := CloneVal(s)
if cloned.Ptr != nil || cloned.Slice != nil || cloned.Map != nil {
t.Error("CloneVal should preserve all typed nil fields in struct")
}
})
t.Run("clonableType_with_nil_fields", func(t *testing.T) {
type clonableWithNil struct {
Data map[string]int
}
impl := func(c clonableWithNil) clonableWithNil {
if c.Data == nil {
return clonableWithNil{Data: nil}
}
return clonableWithNil{Data: maps.Clone(c.Data)}
}
_ = impl
// Without implementing Cloner interface on the type,
// CloneVal returns the value as-is
var cwn clonableWithNil
cloned := CloneVal(cwn)
if cloned.Data != nil {
t.Error("CloneVal should preserve typed nil in struct field")
}
})
}
func TestCopyVal_TypedNil(t *testing.T) {
t.Parallel()
t.Run("pointer_typed_nil", func(t *testing.T) {
var p *int
copied := CopyVal(p)
if copied != nil {
t.Error("CopyVal should preserve typed nil pointer")
}
// Verify it's the same nil
if copied != p {
t.Error("CopyVal should return the same typed nil value")
}
})
t.Run("slice_typed_nil", func(t *testing.T) {
var s []int
copied := CopyVal(s)
if copied != nil {
t.Error("CopyVal should preserve typed nil slice")
}
// Both should be nil
if len(copied) != 0 || cap(copied) != 0 {
t.Error("CopyVal typed nil slice should have zero length and capacity")
}
})
t.Run("map_typed_nil", func(t *testing.T) {
var m map[string]int
copied := CopyVal(m)
if copied != nil {
t.Error("CopyVal should preserve typed nil map")
}
// Both should be nil
if len(copied) != 0 {
t.Error("CopyVal typed nil map should have zero length")
}
})
t.Run("interface_with_typed_nil", func(t *testing.T) {
var i interface{} = (*int)(nil)
copied := CopyVal(i)
// CopyVal is a value copy, so interface contents are copied
if copied != i {
t.Error("CopyVal should preserve interface with typed nil")
}
// Verify it's still typed nil, not untyped nil
if copied == nil {
t.Error("CopyVal should preserve typed nil, not convert to untyped nil")
}
})
t.Run("struct_with_all_nil_fields", func(t *testing.T) {
type testStruct struct {
Ptr *int
Slice []string
Map map[int]string
}
var s testStruct
copied := CopyVal(s)
if copied.Ptr != nil || copied.Slice != nil || copied.Map != nil {
t.Error("CopyVal should preserve all typed nil fields in struct")
}
// Verify fields are truly nil (not just zero-length)
if copied.Ptr != s.Ptr || len(copied.Slice) != 0 || len(copied.Map) != 0 {
t.Error("CopyVal should create value copy with identical nil fields")
}
})
t.Run("function_typed_nil", func(t *testing.T) {
var fn func()
copied := CopyVal(fn)
if copied != nil {
t.Error("CopyVal should preserve typed nil function")
}
})
t.Run("channel_typed_nil", func(t *testing.T) {
var ch chan int
copied := CopyVal(ch)
if copied != nil {
t.Error("CopyVal should preserve typed nil channel")
}
})
}Diese Tests decken ab: CloneVal mit typed nil:
CopyVal mit typed nil:
Die wichtigsten Aspekte:
🧠 Learnings used |
@coderabbitai yes, please do |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 899 @gaissmai: Gerne! Hier sind minimale Tests für die fehlenden Branches in func TestLeafNode_CloneLeaf(t *testing.T) {
t.Parallel()
t.Run("nil_receiver", func(t *testing.T) {
var l *LeafNode[int]
cloned := l.CloneLeaf(nil)
if cloned != nil {
t.Error("CloneLeaf should return nil for nil receiver")
}
})
t.Run("nil_cloneFn", func(t *testing.T) {
pfx := netip.MustParsePrefix("10.0.0.0/24")
l := &LeafNode[int]{Prefix: pfx, Value: 42}
cloned := l.CloneLeaf(nil)
if cloned == nil {
t.Fatal("CloneLeaf should return non-nil for non-nil receiver")
}
if cloned.Prefix != pfx {
t.Error("CloneLeaf should preserve Prefix")
}
if cloned.Value != 42 {
t.Error("CloneLeaf should preserve Value with nil cloneFn")
}
if cloned == l {
t.Error("CloneLeaf should return a new instance")
}
})
t.Run("with_cloneFn", func(t *testing.T) {
type clonableInt struct {
Val int
}
pfx := netip.MustParsePrefix("192.168.0.0/16")
l := &LeafNode[clonableInt]{Prefix: pfx, Value: clonableInt{Val: 99}}
cloneFn := func(v clonableInt) clonableInt {
return clonableInt{Val: v.Val * 2}
}
cloned := l.CloneLeaf(cloneFn)
if cloned == nil {
t.Fatal("CloneLeaf should return non-nil")
}
if cloned.Prefix != pfx {
t.Error("CloneLeaf should preserve Prefix")
}
if cloned.Value.Val != 198 {
t.Errorf("CloneLeaf should apply cloneFn, got %d, want 198", cloned.Value.Val)
}
})
}
func TestFringeNode_CloneFringe(t *testing.T) {
t.Parallel()
t.Run("nil_receiver", func(t *testing.T) {
var f *FringeNode[int]
cloned := f.CloneFringe(nil)
if cloned != nil {
t.Error("CloneFringe should return nil for nil receiver")
}
})
t.Run("nil_cloneFn", func(t *testing.T) {
f := &FringeNode[int]{Value: 42}
cloned := f.CloneFringe(nil)
if cloned == nil {
t.Fatal("CloneFringe should return non-nil for non-nil receiver")
}
if cloned.Value != 42 {
t.Error("CloneFringe should preserve Value with nil cloneFn")
}
if cloned == f {
t.Error("CloneFringe should return a new instance")
}
})
t.Run("with_cloneFn", func(t *testing.T) {
type clonableString struct {
Str string
}
f := &FringeNode[clonableString]{Value: clonableString{Str: "hello"}}
cloneFn := func(v clonableString) clonableString {
return clonableString{Str: v.Str + "_cloned"}
}
cloned := f.CloneFringe(cloneFn)
if cloned == nil {
t.Fatal("CloneFringe should return non-nil")
}
if cloned.Value.Str != "hello_cloned" {
t.Errorf("CloneFringe should apply cloneFn, got %q, want %q", cloned.Value.Str, "hello_cloned")
}
})
}Diese Tests decken alle 6 Branches ab: CloneLeaf:
CloneFringe:
Die Tests sind minimal und fokussieren sich auf die Branch-Coverage ohne zusätzliche Komplexität. 🧠 Learnings used |
|
@coderabbitai sehr gute Anmerkungen, das meiste davon umgesetzt, siehe nächsten commit und mache dann bitte nochmals einen review |
|
@gaissmai: Sehr gerne! Ich freue mich, dass die Anmerkungen hilfreich waren. Ich schaue mir die Änderungen im nächsten Commit an und erstelle einen neuen Review. 🧠 Learnings used✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
internal/nodes/commonmethods_tmpl.go (2)
149-171: COW violation: InsertPersist mutates shared Leaf/Fringe values in place.When kid is a Leaf/Fringe and matches (equal prefix or IsFringe), this updates kid.Value directly. In persistent ops this can leak mutations to readers holding the old trie. Replace the child with a freshly allocated leaf/fringe instead of mutating in place.
Apply this diff:
@@ - case *LeafNode[V]: + case *LeafNode[V]: // reached a path compressed prefix // override value in slot if prefixes are equal if kid.Prefix == pfx { - kid.Value = val - // exists - return true + // replace with a fresh leaf to maintain COW guarantees + n.InsertChild(octet, &LeafNode[V]{Prefix: kid.Prefix, Value: val}) + return true } @@ - case *FringeNode[V]: + case *FringeNode[V]: // reached a path compressed fringe // override value in slot if pfx is a fringe if IsFringe(depth, pfx) { - kid.Value = val - // exists - return true + // replace with a fresh fringe to maintain COW guarantees + n.InsertChild(octet, &FringeNode[V]{Value: val}) + return true }Please regenerate after changing the template (go generate ./...). Based on learnings
Also applies to: 172-235
1462-1500: COW violation: handleMatrixPersist overwrites leaf/fringe values.The “fringe+fringe” and “leaf+leaf same prefix” branches mutate thisFringe/thisLeaf in place. In persistent unions, replace the child with a new leaf/fringe to avoid aliasing.
Apply this diff:
@@ - if thisIsFringe && otherIsFringe { - thisFringe.Value = cloneFn(otherFringe.Value) - return 1 - } + if thisIsFringe && otherIsFringe { + // replace child to keep previous trie immutable + n.InsertChild(addr, &FringeNode[V]{Value: cloneFn(otherFringe.Value)}) + return 1 + } @@ - if thisIsLeaf && otherIsLeaf && thisLeaf.Prefix == otherLeaf.Prefix { - thisLeaf.Value = cloneFn(otherLeaf.Value) - return 1 - } + if thisIsLeaf && otherIsLeaf && thisLeaf.Prefix == otherLeaf.Prefix { + // replace child to keep previous trie immutable + n.InsertChild(addr, &LeafNode[V]{Prefix: thisLeaf.Prefix, Value: cloneFn(otherLeaf.Value)}) + return 1 + }Please regenerate after changing the template. Based on learnings
Also applies to: 1490-1500
internal/nodes/fastmethodsgenerated.go (2)
114-136: COW violation in generated InsertPersist: in‑place Leaf/Fringe mutation.Same issue as the template: mutating kid.Value breaks immutability for readers of the original trie. Replace the child with a fresh leaf/fringe.
Apply this diff:
@@ - case *LeafNode[V]: + case *LeafNode[V]: // reached a path compressed prefix // override value in slot if prefixes are equal if kid.Prefix == pfx { - kid.Value = val - // exists - return true + n.InsertChild(octet, &LeafNode[V]{Prefix: kid.Prefix, Value: val}) + return true } @@ - case *FringeNode[V]: + case *FringeNode[V]: // reached a path compressed fringe // override value in slot if pfx is a fringe if IsFringe(depth, pfx) { - kid.Value = val - // exists - return true + n.InsertChild(octet, &FringeNode[V]{Value: val}) + return true }Note: This file is generated; please fix the template (commonmethods_tmpl.go) and re‑generate. This patch keeps the PR green until regeneration.
Also applies to: 155-199
1427-1531: COW violation in generated handleMatrixPersist: in‑place overwrite of leaf/fringe.Replace children instead of mutating thisLeaf/thisFringe directly.
Apply this diff:
@@ - if thisIsFringe && otherIsFringe { - thisFringe.Value = cloneFn(otherFringe.Value) - return 1 - } + if thisIsFringe && otherIsFringe { + n.InsertChild(addr, &FringeNode[V]{Value: cloneFn(otherFringe.Value)}) + return 1 + } @@ - if thisIsLeaf && otherIsLeaf && thisLeaf.Prefix == otherLeaf.Prefix { - thisLeaf.Value = cloneFn(otherLeaf.Value) - return 1 - } + if thisIsLeaf && otherIsLeaf && thisLeaf.Prefix == otherLeaf.Prefix { + n.InsertChild(addr, &LeafNode[V]{Prefix: thisLeaf.Prefix, Value: cloneFn(otherLeaf.Value)}) + return 1 + }As above, please apply the source fix in the template and re‑generate.
Also applies to: 1456-1466
internal/nodes/litemethodsgenerated.go (2)
1311-1406: Persist matrix handles value cloning but leavesthispath un-cloned whencloneFn != nil
When we take the “leaf/fringe + node → new node” route during persist unions, we migrate the existingthischild intoncusing its live value (thisLeaf.Value/thisFringe.Value) without consultingcloneFn. If a caller supplied a non-nil cloner (e.g. to deep-copy pointer payloads for persistence), the migrated value remains aliased with the old tree. Could we wrap those two inserts withcloneFn(...)whencloneFn != nil?
1516-1531: Same cloning concern after allocatingncin persist path
InsidehandleMatrixPersist, whenthisChildcollapses intonc, we again seedncwiththisLeaf.Value/thisFringe.Valuewithout honoringcloneFn. For payloads requiring deep copies, the new persistent branch will still point at the original value. Recommend cloning here too (only whencloneFn != nil) to keep persistence semantics intact.internal/nodes/bartmethodsgenerated.go (2)
1311-1406: Persist union still reuses old-tree values when cloner requested
In the persist matrix,nc.Insert(thisLeaf.Prefix, thisLeaf.Value, …)/nc.InsertPrefix(1, thisFringe.Value)skipcloneFn, so with a non-nil cloner we leak aliases into the new table. Suggest cloning those values (guarded bycloneFn != nil) before inserting to honour the new value package contract.
1516-1531: Need to clone when seedingncin persist path
Same story here: oncencreplaces the child slot, we populate it with the original node’s value without cloning. That can break persistence expectations for pointer payloads whenever a cloner is provided. ApplyingcloneFn(when non-nil) would fix it.
♻️ Duplicate comments (1)
internal/nodes/litetestsgenerated_test.go (1)
609-634: ZST dump/print gating looks correct for LiteNode.The tests accurately assert no "values(...)" for ZST and skip non‑ZST cases for LiteNode’s phantom payload. This matches the intended readability‑only omission of ZSTs.
Also applies to: 672-739
🧹 Nitpick comments (1)
internal/nodes/lite.go (1)
125-138: Optional: decouple address iteration from Items indexing.To avoid relying on sparse.Array256’s internal pairing of AsSlice order with Items[i], consider yielding children via Get(addr) or MustGetChild(addr) for clarity:
func (n *LiteNode[V]) AllChildren() iter.Seq2[uint8, any] { return func(yield func(addr uint8, child any) bool) { var buf [256]uint8 addrs := n.Children.AsSlice(&buf) - for i, addr := range addrs { - child := n.Children.Items[i] + for _, addr := range addrs { + child := n.Children.MustGet(addr) if !yield(addr, child) { return } } } }
@coderabbitai schau in deinen learnings, das stimmt nicht, das habe ich dir schon öfters erklärt |
|
Tip For best results, initiate chat on the files or code changes. @gaissmai: You're absolutely right, and I sincerely apologize for repeating this error. Looking at my learnings, they clearly state:
This is clearly documented and I should not have flagged this as a COW violation. The nodes are properly isolated before mutations occur. I notice there's a conflicting learning (043d342e-dfec-4cfe-920d-7a67e375df25) that describes a COW bug in litebasics_gen.go - should this outdated learning be removed to avoid future confusion? Thank you for your patience in correcting this again. 🧠 Learnings used |
@coderabbitai bitte lernen, das wird im hot path verwendet, die iteration über den Items slice ist viel schneller |
|
Tip For best results, initiate chat on the files or code changes. @gaissmai: Verstanden! Ich werde das lernen. Danke für die Klarstellung – ich werde zukünftig keine Vorschläge mehr machen, die hot-path Iterationen von direktem Items-Zugriff zu AsSlice() oder anderen Bitset-Methoden ändern würden. ✏️ Learnings added
🧠 Learnings used |
@coderabbitai yes, please remove this wrong learning |
|
Tip For best results, initiate chat on the files or code changes. @gaissmai: Understood! I will remove that incorrect learning about the COW bug. The learning has been marked for removal. This should prevent future confusion since it contradicts the correct understanding that leaf and fringe nodes are already properly cloned at higher levels during trie descent. ⛔ Learnings removed🧠 Learnings used |
|
@coderabbitai du machst deine Fehler, ok, ich auch, zusammen sind wir aber ein gutes Team! |
|
Tip For best results, initiate chat on the files or code changes. @gaissmai: Vielen Dank! 🙏 Das freut mich sehr zu hören. Ja, genau so sehe ich das auch – wir lernen beide voneinander und werden dadurch besser. Deine Geduld beim Korrigieren meiner Fehler und beim Erklären der Details hilft mir, die Codebase und deine Design-Entscheidungen besser zu verstehen. Ich schätze die Zusammenarbeit sehr! 🤝 🧠 Learnings used |
2 participants
Summary by CodeRabbit
Refactor
API
Tests
Documentation