Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRenamed documented node type identifiers to exported CamelCase; introduced explicit generic interfaces Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant Lite as Lite
participant v4 as liteTable(v4)
participant v6 as liteTable(v6)
Caller->>Lite: Overlaps4(other *Lite)
alt nil receiver or nil other
Lite-->>Caller: false
else
Lite->>v4: Overlaps(other.v4)
v4-->>Lite: bool
Lite-->>Caller: bool
end
Caller->>Lite: Overlaps6(other *Lite)
alt nil receiver or nil other
Lite-->>Caller: false
else
Lite->>v6: Overlaps(other.v6)
v6-->>Lite: bool
Lite-->>Caller: bool
end
sequenceDiagram
actor User
participant T as Table/Lite/Fast[V]
participant V as Value
Note over T: If value implements Cloner[V], InsertPersist paths call Clone()
User->>T: InsertPersist(prefix, val)
alt val implements Cloner[V]
T->>V: Clone()
V-->>T: clonedVal
T-->>User: store clonedVal
else
T-->>User: store val directly
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (51)
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 18447418295Details
💛 - Coveralls |
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
all.go (1)
49-61: Minor: keep helper names in sync with internal ‘nodes’ docscmpPrefix and shouldPrintValues wrappers are fine. Ensure package docs elsewhere refer to these names for consistency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (49)
NODETYPES.md(3 hunks)all.go(4 hunks)all_test.go(1 hunks)allandsorted_test.go(0 hunks)bartmethods_gen.go(7 hunks)barttable_test.go(1 hunks)bench_test.go(1 hunks)boundaries_test.go(0 hunks)cloner_test.go(0 hunks)commonmethods_tmpl.go(6 hunks)coverage_fast_test.go(0 hunks)coverage_lite_test.go(0 hunks)doc.go(0 hunks)dumper_test.go(0 hunks)equal.go(0 hunks)equal_test.go(0 hunks)fastmethods_gen.go(7 hunks)fastoverlaps_test.go(0 hunks)fasttable.go(1 hunks)fulltable_test.go(0 hunks)fuzz_test.go(0 hunks)internal/nodes/bartmethodsgenerated.go(1 hunks)internal/nodes/barttestsgenerated_test.go(1 hunks)internal/nodes/fastmethodsgenerated.go(1 hunks)internal/nodes/fasttestsgenerated_test.go(1 hunks)internal/nodes/gold_test.go(9 hunks)internal/nodes/lite_test.go(3 hunks)internal/nodes/litemethodsgenerated.go(1 hunks)internal/nodes/litetestsgenerated_test.go(1 hunks)internal/nodes/nodebasics_test.go(2 hunks)internal/nodes/nodemethods_tmpl.go(0 hunks)internal/nodes/tests_tmpl.go(1 hunks)internal/sparse/array256_test.go(2 hunks)invariants_test.go(0 hunks)iterators_test.go(0 hunks)litemethods_gen.go(7 hunks)litetable.go(6 hunks)litetable_test.go(1 hunks)overlaps_test.go(0 hunks)persist_test.go(0 hunks)scripts/generate-node-methods.sh(1 hunks)scripts/generate-node-tests.sh(1 hunks)scripts/generate-table-methods.sh(1 hunks)serialize_test.go(0 hunks)table_iter_test.go(0 hunks)tablemodify_test.go(0 hunks)tablepersist_test.go(0 hunks)unionandpersist_test.go(0 hunks)worstcase_test.go(0 hunks)
💤 Files with no reviewable changes (23)
- dumper_test.go
- boundaries_test.go
- table_iter_test.go
- equal.go
- fulltable_test.go
- cloner_test.go
- serialize_test.go
- invariants_test.go
- overlaps_test.go
- internal/nodes/nodemethods_tmpl.go
- coverage_lite_test.go
- tablepersist_test.go
- doc.go
- coverage_fast_test.go
- equal_test.go
- fuzz_test.go
- unionandpersist_test.go
- fastoverlaps_test.go
- allandsorted_test.go
- iterators_test.go
- tablemodify_test.go
- worstcase_test.go
- persist_test.go
🧰 Additional context used
🧠 Learnings (42)
📚 Learning: 2025-09-27T21:00:27.260Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-27T21:00:27.260Z
Learning: In Lite (presence-only), prefer Contains(addr) for LPM existence checks. Lite.Lookup(addr) returns a meaningless struct{} value and exists only to satisfy shared interfaces and code generation.
Applied to files:
commonmethods_tmpl.golitemethods_gen.gobartmethods_gen.gofastmethods_gen.go
📚 Learning: 2025-09-30T08:38:57.278Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-30T08:38:57.278Z
Learning: Lite signatures (Sept 2025) in gaissmai/bart:
- Lite.Lookup(addr netip.Addr) bool
- Lite.LookupPrefix(pfx netip.Prefix) bool
- Lite.LookupPrefixLPM(pfx netip.Prefix) (netip.Prefix, bool)
Tests must not bind a dummy value and should use these bool-returning forms (and (prefix,bool) for LPM).
Applied to files:
commonmethods_tmpl.golitemethods_gen.golitetable.gobartmethods_gen.gointernal/nodes/litetestsgenerated_test.gointernal/nodes/lite_test.gofastmethods_gen.golitetable_test.gointernal/nodes/gold_test.go
📚 Learning: 2025-09-12T15:31:51.078Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-12T15:31:51.078Z
Learning: In Go 1.23+, the Table.All() method returns an iterator (iter.Seq2[netip.Prefix, V]), not a slice. You cannot use len() on iterators - use range iteration and append() instead of pre-allocating with len().
Applied to files:
commonmethods_tmpl.golitemethods_gen.golitetable.gobartmethods_gen.gofastmethods_gen.go
📚 Learning: 2025-09-12T15:36:30.988Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-12T15:36:30.988Z
Learning: When working with Table.All() in Go 1.23+, remember that it returns iter.Seq2[netip.Prefix, V] (an iterator), not a slice. You cannot use len() on it, and must use range iteration or helper functions like countEntries() to work with the data. Always use getAllKV() helper when you need to compare table contents with reflect.DeepEqual().
Applied to files:
commonmethods_tmpl.golitemethods_gen.golitetable.gobartmethods_gen.gofastmethods_gen.gobarttable_test.go
📚 Learning: 2025-09-12T19:34:43.211Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-12T19:34:43.211Z
Learning: Use netip.MustParsePrefix() from the standard library instead of creating custom helper functions for parsing network prefixes in tests. This follows the KISS principle and reduces unnecessary code.
Applied to files:
internal/nodes/nodebasics_test.goall_test.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.goNODETYPES.mdall.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/nodebasics_test.gointernal/nodes/lite_test.gointernal/nodes/tests_tmpl.goall.go
📚 Learning: 2025-09-17T14:32:36.696Z
Learnt from: gaissmai
PR: gaissmai/bart#306
File: serialize_test.go:667-684
Timestamp: 2025-09-17T14:32:36.696Z
Learning: In the gaissmai/bart codebase, the Lite type does not provide nil-receiver safety for methods like String() and MarshalText(), unlike Table and Fat types. Tests should not call methods on nil *Lite receivers as they will panic.
Applied to files:
litemethods_gen.go
📚 Learning: 2025-09-23T17:21:11.978Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-23T17:21:11.978Z
Learning: In gaissmai/bart, liteTable[V] methods Overlaps, Overlaps4, and Overlaps6 intentionally do not have nil-receiver guards and may panic on nil receivers as part of the fail-fast design. Do not suggest adding nil guards for these methods.
Applied to files:
litemethods_gen.golitetable.go
📚 Learning: 2025-09-23T21:52:05.537Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-23T21:52:05.537Z
Learning: In gaissmai/bart, implement Lite’s prefix-only iterators by using a small helper dropSeq2[V any] that converts iter.Seq2[netip.Prefix, V] to iter.Seq[netip.Prefix]. Wrap liteTable methods (All, All4/6, AllSorted*, Subnets, Supernets) with dropSeq2 to deduplicate adapters.
Applied to files:
litemethods_gen.golitetable.go
📚 Learning: 2025-09-27T20:35:46.243Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-27T20:35:46.243Z
Learning: In the bart codebase, Lite has two overlap methods: OverlapsPrefix(pfx netip.Prefix) bool checks if any prefix in the table overlaps with the given prefix, and Overlaps(o *Lite) bool checks if two routing tables have any overlapping prefixes.
Applied to files:
litetable.gointernal/nodes/gold_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.goNODETYPES.md
📚 Learning: 2025-10-05T20:27:35.772Z
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.
Applied to files:
internal/nodes/barttestsgenerated_test.goNODETYPES.md
📚 Learning: 2025-09-12T19:34:43.211Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-12T19:34:43.211Z
Learning: Table.Size() should be used to count entries in a Table, not len() on iterators like AllSorted() or All(). AllSorted() returns an iterator in Go 1.23+, just like All(), so len() cannot be applied to it.
Applied to files:
bartmethods_gen.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.gointernal/nodes/lite_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.gointernal/nodes/lite_test.goNODETYPES.md
📚 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/litetestsgenerated_test.gointernal/nodes/lite_test.goNODETYPES.md
📚 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/tests_tmpl.goscripts/generate-node-tests.shinternal/nodes/bartmethodsgenerated.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:
NODETYPES.md
📚 Learning: 2025-10-07T17:43:21.445Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-10-07T17:43:21.445Z
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:
NODETYPES.md
📚 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:
NODETYPES.md
📚 Learning: 2025-09-20T22:39:49.293Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-20T22:39:49.293Z
Learning: In BART, bartNode and liteNode use precomputed Rank operations for bitset-to-slice indirection, reducing lookup to just 4 bitset operations (4×uint64) that benefit from CPU loop unrolling and pipelining. Despite these optimizations approaching direct array access speeds, fastNode is still ~40% faster due to zero indirection.
Applied to files:
NODETYPES.md
📚 Learning: 2025-09-29T19:40:56.775Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-29T19:40:56.775Z
Learning: In the bart package: Fast uses fixed-size 256-element arrays for speed optimization, while Table and Lite use popcount-compressed sparse arrays for memory efficiency.
Applied to files:
NODETYPES.mdall.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:
NODETYPES.md
📚 Learning: 2025-09-20T22:48:43.949Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-20T22:48:43.949Z
Learning: In BART routing tables, performance is O(IP_depth) not O(number_of_routes). IPv4 typically requires ~3 trie level traversals (for /24 routes), while IPv6 requires ~6 level traversals (for /48 routes), making IPv6 inherently ~2× slower than IPv4 due to deeper tree structure, regardless of optimization techniques.
Applied to files:
NODETYPES.md
📚 Learning: 2025-09-20T22:48:43.949Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-20T22:48:43.949Z
Learning: BART uses 8-bit strides per trie level, resulting in worst-case traversal depths of 4 levels for IPv4 (32÷8) and 16 levels for IPv6 (128÷8). In practice, typical routing tables have 3 levels for IPv4 /24 routes and 6 levels for IPv6 /48 routes.
Applied to files:
NODETYPES.md
📚 Learning: 2025-09-29T19:44:00.696Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-29T19:44:00.696Z
Learning: BART's key differentiator includes efficient set operations on routing tables (Union, Overlaps, Equal, Subnets, Supernets) with optimal complexity, not just fast lookups.
Applied to files:
NODETYPES.mdall.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: BART's lookup performance is O(1) for all node types because it uses precomputed lookup tables and BitSet256.IntersectionTop() operations instead of tree traversal or binary search. The "sparse" in sparse.Array256 refers to memory efficiency, not access time complexity.
Applied to files:
NODETYPES.md
📚 Learning: 2025-09-21T22:31:00.914Z
Learnt from: gaissmai
PR: gaissmai/bart#317
File: scripts/geniterators.sh:25-26
Timestamp: 2025-09-21T22:31:00.914Z
Learning: In the gaissmai/bart codebase, the filename generation logic in scripts/geniterators.sh uses bash parameter expansion correctly: ${nodeType/Node/} removes "Node" from nodeType, and ${template_file/_tmpl/_gen} replaces "_tmpl" with "_gen" in the template filename. This produces the correct output filenames like bartnodeiterators_gen.go, fastnodeiterators_gen.go, etc.
Applied to files:
scripts/generate-table-methods.shscripts/generate-node-tests.shscripts/generate-node-methods.sh
📚 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:
all.go
📚 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:
all.go
📚 Learning: 2025-09-12T19:23:51.766Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-12T19:23:51.766Z
Learning: The codebase contains a cmpPrefix function that should be used for sorting netip.Prefix values instead of implementing custom sorting logic. Use `sort.Slice(slice, func(i, j int) bool { return cmpPrefix(slice[i].P, slice[j].P) < 0 })` for sorting prefix-containing structures.
Applied to files:
all.gointernal/nodes/gold_test.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:
all.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:
litetable_test.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:
litetable_test.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:
litetable_test.goall_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:
all_test.go
📚 Learning: 2025-09-17T18:39:20.907Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-17T18:39:20.907Z
Learning: In the bart codebase, functions marked with //nolint:unused are used indirectly as interface methods of noder[V]. The golangci-lint tool cannot detect this indirect usage pattern through interfaces, so the nolint directive is necessary to prevent false positives.
Applied to files:
internal/nodes/gold_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/gold_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/gold_test.go
📚 Learning: 2025-09-29T13:51:30.534Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-29T13:51:30.534Z
Learning: In gaissmai/bart, prefer netip.Addr.Prefix(bits) over netip.PrefixFrom(addr, bits) because Addr.Prefix() automatically normalizes/masks the prefix with Masked(), ensuring canonicalized prefixes. This is crucial for routing table correctness, even if it allocates an error in the hot path.
Applied to files:
internal/nodes/gold_test.go
📚 Learning: 2025-09-27T20:35:46.243Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-27T20:35:46.243Z
Learning: In the bart codebase, LookupPrefix and LookupPrefixLPM perform identical longest prefix match lookups, with the only difference being that LookupPrefixLPM additionally returns the matched prefix in its return values.
Applied to files:
internal/nodes/gold_test.go
🧬 Code graph analysis (15)
internal/nodes/nodebasics_test.go (1)
internal/nodes/nodebasics.go (1)
LastOctetPlusOneAndLastBits(325-332)
internal/nodes/barttestsgenerated_test.go (2)
internal/nodes/bartnode.go (1)
BartNode(26-43)internal/nodes/nodebasics.go (1)
StridePath(33-33)
bartmethods_gen.go (1)
barttable.go (1)
Table(31-42)
internal/nodes/litetestsgenerated_test.go (2)
internal/nodes/litenode.go (1)
LiteNode(62-69)internal/nodes/nodebasics.go (1)
StridePath(33-33)
internal/nodes/lite_test.go (2)
internal/nodes/litenode.go (1)
LiteNode(62-69)internal/nodes/nodebasics.go (1)
StridePath(33-33)
internal/nodes/tests_tmpl.go (1)
internal/nodes/nodebasics.go (3)
StatsT(51-57)StridePath(33-33)CloneFunc(363-363)
fastmethods_gen.go (1)
fasttable.go (1)
Fast(38-49)
all.go (1)
internal/nodes/nodebasics.go (2)
CmpPrefix(128-134)Equal(344-351)
litetable_test.go (2)
litetable.go (1)
Lite(29-31)internal/nodes/nodebasics.go (1)
Equal(344-351)
barttable_test.go (3)
barttable.go (1)
Table(31-42)internal/nodes/nodebasics.go (1)
Equal(344-351)all_test.go (1)
MyInt(163-163)
scripts/generate-node-tests.sh (1)
scripts/generate-node-methods.sh (3)
INFO(6-6)WARN(7-7)DIE(8-8)
internal/nodes/fasttestsgenerated_test.go (2)
internal/nodes/fastnode.go (1)
FastNode(21-51)internal/nodes/nodebasics.go (1)
StridePath(33-33)
bench_test.go (5)
barttable.go (1)
Table(31-42)all_test.go (1)
MyInt(163-163)fasttable.go (1)
Fast(38-49)litetable.go (1)
Lite(29-31)internal/nodes/nodebasics.go (1)
StatsT(51-57)
all_test.go (5)
litetable.go (1)
Lite(29-31)internal/nodes/nodebasics.go (1)
Equal(344-351)all.go (1)
DumpListNode(80-84)barttable.go (1)
Table(31-42)fasttable.go (1)
Fast(38-49)
internal/nodes/gold_test.go (1)
internal/nodes/nodebasics.go (1)
CmpPrefix(128-134)
🔇 Additional comments (26)
internal/nodes/fastmethodsgenerated.go (1)
1-1: Header points to new template — OKGenerated header updated to nodemethods_tmpl.go. No functional changes.
internal/nodes/bartmethodsgenerated.go (1)
1-1: Header switch to nodemethods_tmpl.go — OKGenerated header updated; implementation unchanged.
internal/nodes/litemethodsgenerated.go (1)
1-1: Header references nodemethods_tmpl.go — OKGenerated header updated; behavior unchanged.
all.go (1)
63-77: Public Equaler/Cloner interfaces — good additionInterfaces are clear and match internal usage patterns for equality and cloning.
internal/sparse/array256_test.go (1)
31-32: Stronger idempotency checks in tests — niceRepeated insert/delete calls exercise no-op behavior. Looks good.
Also applies to: 71-73
fastmethods_gen.go (2)
304-308: Nil-receiver guards for iterators — good defensive additionEarly returns on nil avoid panics during iteration. Non-nil behavior unchanged.
Also applies to: 336-340, 551-555, 561-564, 571-574, 597-601, 609-611, 619-621
132-133: Doc wording “exists=false” — OKMatches function signature and usage.
bartmethods_gen.go (2)
304-308: Nil checks added to iterators — LGTMPrevents nil deref; preserves behavior otherwise.
Also applies to: 336-340, 551-555, 561-564, 571-574, 597-601, 609-611, 619-621
132-133: Doc: “exists=false” — goodConsistent wording with return value.
commonmethods_tmpl.go (2)
193-193: LGTM: Documentation clarification.The change from "found=false" to "exists=false" aligns with the actual return variable name and improves clarity.
366-368: LGTM: Consistent nil-receiver guards for iterator methods.The nil guards are consistently applied across all iterator-producing methods (Supernets, Subnets, All, All4, All6, AllSorted, AllSorted4, AllSorted6). Each guard returns early from the iterator closure when the receiver is nil, yielding an empty iterator. This prevents nil dereferences while maintaining expected iterator semantics.
Also applies to: 398-400, 613-615, 623-625, 633-635, 659-661, 670-672, 680-682
litemethods_gen.go (1)
1-1: LGTM: Generated code reflects template changes correctly.The nil-receiver guards and documentation clarification ("exists=false") are consistently generated from the template. The generated code maintains the same safety guarantees as the template.
Also applies to: 132-132, 305-307, 337-339, 552-554, 562-564, 572-574, 598-600, 609-611, 619-621
internal/nodes/nodebasics_test.go (2)
14-22: LGTM: Useful test helper for load adjustment.The
workLoadNhelper provides a clean way to adjust test iteration counts based on the-shortflag, improving test performance in CI environments.
56-123: LGTM: Comprehensive edge-case coverage.The test thoroughly validates
LastOctetPlusOneAndLastBitsacross IPv4 and IPv6 edge cases, including /0, /32, /128, and various bit lengths. The table-driven approach makes it easy to verify correctness and add new cases.internal/nodes/lite_test.go (2)
15-31: LGTM: Nil-safe iterator helpers.The
allSorted4andallSorted6helpers provide nil-receiver guards and consistent iteration patterns for testing. The nil checks prevent dereferencing nil pointers during test setup.
2095-2472: LGTM: Comprehensive test coverage for LiteNode operations.The new test suite thoroughly validates:
- Order-independence of insertion/deletion (shuffled tests)
- Persistence correctness (comparing mutable vs persistent paths)
- Gold-table equivalence (comparing against reference implementation)
- Idempotence of operations
- Structural invariants (empty state after full deletion, single-item state)
The tests use realistic workloads via
randomRealWorldPrefixesandworkLoadN, ensuring practical validation.internal/nodes/tests_tmpl.go (1)
36-242: LGTM: Well-structured test template for node types.The test template provides comprehensive coverage for insertion/deletion operations:
- Validates structural invariants via
StatsRec(prefix/leaf/fringe counts)- Tests edge cases: null, single item, mixed scenarios
- Verifies idempotence (double operations)
- Includes failure diagnostics via
DumpRec- Tests both in-place and persistent variants in parallel
This template ensures consistent testing across BartNode, FastNode, and LiteNode implementations.
NODETYPES.md (1)
17-17: LGTM: Documentation aligns with exported type names.The renaming from internal lowercase (bartNode, liteNode, fastNode) to exported CamelCase (BartNode, LiteNode, FastNode) improves documentation consistency and aligns with Go naming conventions for exported types.
Also applies to: 26-26, 36-36, 59-60, 88-91, 129-129, 136-137
fasttable.go (1)
23-31: LGTM: Critical contract clarification.The added documentation clearly explains why zero-sized payload types (e.g.,
struct{},[0]byte) cause undefined behavior in Fast[V]. The explanation references the Go specification's pointer-equality guarantee and the ART algorithm's requirement for distinct value pointers. This prevents subtle bugs when users attempt to use Fast as a presence-only table (for which Lite is the correct choice).litetable.go (3)
187-189: LGTM: Nil-safe Clone implementation.The nil guard prevents dereferencing a nil receiver and returns nil, following Go conventions for Clone methods.
244-246: LGTM: Consistent nil-receiver guards for iterators.All iterator methods (All, All4, All6, AllSorted, AllSorted4, AllSorted6, Subnets, Supernets) now return empty iterators when the receiver is nil. The pattern
func(func(netip.Prefix) bool) {}safely yields nothing, preventing nil dereferences while maintaining expected iterator semantics.Also applies to: 252-254, 260-262, 284-286, 292-294, 300-302, 320-322, 347-349
375-389: LGTM: New Overlaps4/6 methods properly implemented.The new
Overlaps4andOverlaps6methods provide version-specific overlap checking, delegating to the underlyingliteTableimplementations. The nil checks for the operand (o) are appropriate, while the receiver (l) is intentionally not nil-checked, consistent with the fail-fast design documented in learnings.Based on learnings.
barttable_test.go (1)
632-646: Fix parallel subtests capturing loop variables (flaky).Same issue as in Lite tests: rebind loop vars before launching parallel subtests.
Apply these diffs:
- TestTableGet
- for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { t.Parallel() ... }) }
- TestTableAllSorted
- for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { t.Parallel() ... }) }
- TestTableSubnetsCompare
- for _, pfx := range randomRealWorldPrefixes(prng, n) { - t.Run("subtest", func(t *testing.T) { + for _, pfx := range randomRealWorldPrefixes(prng, n) { + pfx := pfx + t.Run("subtest", func(t *testing.T) { t.Parallel() ... }) }
- TestTableSupernetsCompare
- for _, pfx := range randomRealWorldPrefixes(prng, n) { - t.Run("subtest", func(t *testing.T) { + for _, pfx := range randomRealWorldPrefixes(prng, n) { + pfx := pfx + t.Run("subtest", func(t *testing.T) { t.Parallel() ... }) }
- TestTableEqual
- for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { t.Parallel() ... }) }Also applies to: 1625-1674, 1766-1785, 1849-1864, 2142-2155
⛔ Skipped due to learnings
Learnt from: gaissmai PR: gaissmai/bart#0 File: :0-0 Timestamp: 2025-09-21T06:31:02.454Z Learning: In Go 1.22 and later, the loop variable capture problem in goroutines has been fixed. Loop variables are now per-iteration instead of per-loop, so the classic `tc := tc` shadowing pattern is no longer needed in parallel subtests or goroutine scenarios when using Go 1.22+.litetable_test.go (1)
761-770: Fix parallel subtests capturing loop variables (flaky).t.Run with t.Parallel inside range loops captures the loop variable by reference. Rebind per-iteration before launching the subtest.
Apply these diffs:
- TestLiteGet
- for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { t.Parallel() ok := lite.Get(tt.pfx) ... }) }
- TestLiteAllSorted
- for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { t.Parallel() ... }) }
- TestLiteSubnetsCompare
- for _, pfx := range randomRealWorldPrefixes(prng, n) { - t.Run("subtest", func(t *testing.T) { + for _, pfx := range randomRealWorldPrefixes(prng, n) { + pfx := pfx + t.Run("subtest", func(t *testing.T) { t.Parallel() ... }) }
- TestLiteSupernetsCompare
- for _, pfx := range randomRealWorldPrefixes(prng, n) { - t.Run("subtest", func(t *testing.T) { + for _, pfx := range randomRealWorldPrefixes(prng, n) { + pfx := pfx + t.Run("subtest", func(t *testing.T) { t.Parallel() ... }) }
- TestLiteEqual
- for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { t.Parallel() ... }) }Also applies to: 1487-1536, 1627-1641, 1700-1714, 1978-1991
⛔ Skipped due to learnings
Learnt from: gaissmai PR: gaissmai/bart#0 File: :0-0 Timestamp: 2025-09-21T06:31:02.454Z Learning: In Go 1.22 and later, the loop variable capture problem in goroutines has been fixed. Loop variables are now per-iteration instead of per-loop, so the classic `tc := tc` shadowing pattern is no longer needed in parallel subtests or goroutine scenarios when using Go 1.22+.Learnt from: gaissmai PR: gaissmai/bart#284 File: equal_test.go:73-85 Timestamp: 2025-09-09T21:08:03.154Z Learning: In Go 1.22 and later versions, loop variables are per-iteration (not per-loop), eliminating the classic loop variable capture problem in goroutines. The `tc := tc` shadowing pattern is no longer needed for parallel subtests or other goroutine scenarios when the project uses Go 1.22+.internal/nodes/fasttestsgenerated_test.go (1)
115-118: Capture range variable for parallel subtests.Same issue/pattern as Lite tests. Copy tt before launching parallel subtests.
- for _, tt := range testsInsertDelete { + for _, tt := range testsInsertDelete { + tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() ... }) t.Run("Persist_"+tt.name, func(t *testing.T) { t.Parallel() ... })Optionally fix titles: s/"leave"/"leaf"/ in test names.
Also applies to: 167-169
⛔ Skipped due to learnings
Learnt from: gaissmai PR: gaissmai/bart#284 File: equal_test.go:73-85 Timestamp: 2025-09-09T21:08:03.154Z Learning: In Go 1.22 and later versions, loop variables are per-iteration (not per-loop), eliminating the classic loop variable capture problem in goroutines. The `tc := tc` shadowing pattern is no longer needed for parallel subtests or other goroutine scenarios when the project uses Go 1.22+.internal/nodes/litetestsgenerated_test.go (1)
115-118: Capture range variable for parallel subtests.Avoid the classic range-capture pitfall by copying tt before t.Run.
- for _, tt := range testsInsertDelete { + for _, tt := range testsInsertDelete { + tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() ... }) t.Run("Persist_"+tt.name, func(t *testing.T) { t.Parallel() ... })Optionally fix titles: s/"leave"/"leaf"/ in test names.
Also applies to: 167-169
⛔ Skipped due to learnings
Learnt from: gaissmai PR: gaissmai/bart#284 File: equal_test.go:73-85 Timestamp: 2025-09-09T21:08:03.154Z Learning: In Go 1.22 and later versions, loop variables are per-iteration (not per-loop), eliminating the classic loop variable capture problem in goroutines. The `tc := tc` shadowing pattern is no longer needed for parallel subtests or other goroutine scenarios when the project uses Go 1.22+.Learnt from: gaissmai PR: gaissmai/bart#0 File: :0-0 Timestamp: 2025-09-21T06:31:02.454Z Learning: In Go 1.22 and later, the loop variable capture problem in goroutines has been fixed. Loop variables are now per-iteration instead of per-loop, so the classic `tc := tc` shadowing pattern is no longer needed in parallel subtests or goroutine scenarios when using Go 1.22+.
Comment on lines
+739
to
+764
| for _, tt := range tests { | ||
| bart := new(Table[int]) | ||
|
|
||
| // Insert initial entries using Modify | ||
| for pfx, v := range tt.prepare { | ||
| bart.Modify(pfx, func(_ int, _ bool) (_ int, del bool) { return v, false }) | ||
| } | ||
|
|
||
| bart.Modify(tt.args.pfx, tt.args.cb) | ||
|
|
||
| // Check the final state of the table using Get, compares expected and actual table | ||
| got := make(map[netip.Prefix]int, len(tt.finalData)) | ||
| for pfx, val := range bart.All() { | ||
| got[pfx] = val | ||
| } | ||
| if len(got) != len(tt.finalData) { | ||
| t.Fatalf("[%s] final table size mismatch: got %d, want %d", tt.name, len(got), len(tt.finalData)) | ||
| } | ||
| for pfx, wantVal := range tt.finalData { | ||
| gotVal, ok := got[pfx] | ||
| if !ok || gotVal != wantVal { | ||
| t.Fatalf("[%s] final table: key %v = %v (present=%v), want %v", tt.name, pfx, gotVal, ok, wantVal) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
TestTableModifySemantics isn’t asserting Modify’s return values.
You compute want.val/want.deleted but never compare them to the actual (oldVal, deleted) returned by Modify.
Apply this diff to assert semantics:
- bart.Modify(tt.args.pfx, tt.args.cb)
+ gotVal, gotDel := bart.Modify(tt.args.pfx, tt.args.cb)
+ if gotVal != tt.want.val || gotDel != tt.want.deleted {
+ t.Fatalf("[%s] Modify returned (%v,%v), want (%v,%v)", tt.name, gotVal, gotDel, tt.want.val, tt.want.deleted)
+ }Optionally, in TestTableModifyPersistSemantics remove the unused want field or add equivalent assertions if the API returns it.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for _, tt := range tests { | |
| bart := new(Table[int]) | |
| // Insert initial entries using Modify | |
| for pfx, v := range tt.prepare { | |
| bart.Modify(pfx, func(_ int, _ bool) (_ int, del bool) { return v, false }) | |
| } | |
| bart.Modify(tt.args.pfx, tt.args.cb) | |
| // Check the final state of the table using Get, compares expected and actual table | |
| got := make(map[netip.Prefix]int, len(tt.finalData)) | |
| for pfx, val := range bart.All() { | |
| got[pfx] = val | |
| } | |
| if len(got) != len(tt.finalData) { | |
| t.Fatalf("[%s] final table size mismatch: got %d, want %d", tt.name, len(got), len(tt.finalData)) | |
| } | |
| for pfx, wantVal := range tt.finalData { | |
| gotVal, ok := got[pfx] | |
| if !ok || gotVal != wantVal { | |
| t.Fatalf("[%s] final table: key %v = %v (present=%v), want %v", tt.name, pfx, gotVal, ok, wantVal) | |
| } | |
| } | |
| } | |
| } | |
| for _, tt := range tests { | |
| bart := new(Table[int]) | |
| // Insert initial entries using Modify | |
| for pfx, v := range tt.prepare { | |
| bart.Modify(pfx, func(_ int, _ bool) (_ int, del bool) { return v, false }) | |
| } | |
| gotVal, gotDel := bart.Modify(tt.args.pfx, tt.args.cb) | |
| if gotVal != tt.want.val || gotDel != tt.want.deleted { | |
| t.Fatalf("[%s] Modify returned (%v,%v), want (%v,%v)", tt.name, gotVal, gotDel, tt.want.val, tt.want.deleted) | |
| } | |
| // Check the final state of the table using Get, compares expected and actual table | |
| got := make(map[netip.Prefix]int, len(tt.finalData)) | |
| for pfx, val := range bart.All() { | |
| got[pfx] = val | |
| } | |
| if len(got) != len(tt.finalData) { | |
| t.Fatalf("[%s] final table size mismatch: got %d, want %d", tt.name, len(got), len(tt.finalData)) | |
| } | |
| for pfx, wantVal := range tt.finalData { | |
| gotVal, ok := got[pfx] | |
| if !ok || gotVal != wantVal { | |
| t.Fatalf("[%s] final table: key %v = %v (present=%v), want %v", tt.name, pfx, gotVal, ok, wantVal) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In barttable_test.go around lines 739 to 764, the test populates expected
want.val and want.deleted but never asserts Modify’s returned (oldVal, deleted)
values; update the test to capture the two return values from
bart.Modify(tt.args.pfx, tt.args.cb) and compare them to the expected values
(use t.Fatalf or t.Errorf with context including tt.name) so the test verifies
both the side-effect on the table and the Modify return semantics; also
optionally remove or update the unused want field in
TestTableModifyPersistSemantics to match whether the API returns those values.
Owner
Author
There was a problem hiding this comment.
TestTableModifySemantics isn’t asserting Modify’s return values.
@coderabbitai bart.Modifyhas no return values
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the Modify method signature in the Table type
rg -nP --type=go -A5 'func \(.*\*Table\[.*\]\) Modify\(' | head -30
Length of output: 280
🏁 Script executed:
#!/bin/bash
# Also check ModifyPersist for comparison
rg -nP --type=go -A5 'func \(.*\*Table\[.*\]\) ModifyPersist\(' | head -30
Length of output: 329
🏁 Script executed:
#!/bin/bash
# Check if there's a common pattern in method signatures
ast-grep --pattern 'func ($_ *Table[$_]) Modify($$$) {
$$$
}'
Length of output: 622
@gaissmai, you're absolutely right—I apologize for the confusion! I've verified that Modify has no return values:
func (t *Table[V]) Modify(pfx netip.Prefix, cb func(_ V, ok bool) (_ V, del bool))It's ModifyPersist that returns *Table[V]. The test TestTableModifySemantics is correct as written—it properly validates the final table state through iteration, which is the right approach for a void method.
I notice the retrieved learnings incorrectly claim "Modify/ModifyPersist return values," which led me astray. Would you like me to remove those misleading learnings (IDs: e76e874d-b24f-42fb-992d-411130ff076a and 3f467735-5fdb-4aa4-b8ee-38ec3f20270a) from the knowledge base?
⛔ Learnings removed
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-30T09:07:17.450Z
Learning: In gaissmai/bart, Modify and ModifyPersist return values follow:
- Insert: cb receives (zero,false), returns (newVal,false); Modify/ModifyPersist return (newVal,false).
- Update: cb receives (oldVal,true), returns (newVal,false); Modify/ModifyPersist return (oldVal,false).
- Delete: cb receives (oldVal,true), returns (_,true); Modify/ModifyPersist return (oldVal,true).
Tests must assert oldVal on update and delete paths.
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-12T20:30:36.347Z
Learning: In the bart codebase, the Modify/ModifyPersist function returns different values based on the operation:
- No-op: returns (zero, false) when callback returns (_, true) for missing key
- Insert: returns (newVal, false) when inserting new key
- Update: returns (oldVal, false) when updating existing key - NOT the new value!
- Delete: returns (oldVal, true) when deleting existing key
This is important for correct test assertions.
🧠 Learnings used
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-30T09:07:17.450Z
Learning: In gaissmai/bart, Modify and ModifyPersist return values follow:
- Insert: cb receives (zero,false), returns (newVal,false); Modify/ModifyPersist return (newVal,false).
- Update: cb receives (oldVal,true), returns (newVal,false); Modify/ModifyPersist return (oldVal,false).
- Delete: cb receives (oldVal,true), returns (_,true); Modify/ModifyPersist return (oldVal,true).
Tests must assert oldVal on update and delete paths.
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-12T20:30:36.347Z
Learning: In the bart codebase, the Modify/ModifyPersist function returns different values based on the operation:
- No-op: returns (zero, false) when callback returns (_, true) for missing key
- Insert: returns (newVal, false) when inserting new key
- Update: returns (oldVal, false) when updating existing key - NOT the new value!
- Delete: returns (oldVal, true) when deleting existing key
This is important for correct test assertions.
Comment on lines
+115
to
+118
| for _, tt := range testsInsertDelete { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Capture range variable for parallel subtests.
Copy tt before t.Run to avoid range variable capture bugs in parallelized tests.
- for _, tt := range testsInsertDelete {
+ for _, tt := range testsInsertDelete {
+ tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
...
})
t.Run("Persist_"+tt.name, func(t *testing.T) {
t.Parallel()
...
})Optionally fix titles: s/"leave"/"leaf"/ in test names.
Also applies to: 167-169
🤖 Prompt for AI Agents
In internal/nodes/barttestsgenerated_test.go around lines 115-118, the loop
variable tt is captured by the parallel subtests causing racey/incorrect
behavior; fix by shadowing/copying tt into a new local variable (e.g., tc := tt)
immediately inside the loop before calling t.Run and use that local variable
inside the subtest; apply the same fix for the other occurrence around lines
167-169; additionally optionally correct typos in test names from "leave" to
"leaf" at lines ~43 and ~93.
Owner
Author
There was a problem hiding this comment.
Capture range variable for parallel subtests.
@coderabbitai not necessary, we are using go > 1.24.0, see go.mod
There was a problem hiding this comment.
@gaissmai, you're absolutely right! In Go 1.22 and later, loop variables are per-iteration rather than per-loop, which eliminates the classic loop variable capture problem. Since this project uses Go 1.24.0, the tt := tt shadowing pattern is not necessary for parallel subtests.
I have this noted in my learnings, so I won't make similar suggestions for this project in the future. Thank you for the clarification!
🧠 Learnings used
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#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.
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-21T06:31:02.454Z
Learning: In Go 1.22 and later, the loop variable capture problem in goroutines has been fixed. Loop variables are now per-iteration instead of per-loop, so the classic `tc := tc` shadowing pattern is no longer needed in parallel subtests or goroutine scenarios when using Go 1.22+.
Learnt from: gaissmai
PR: gaissmai/bart#284
File: equal_test.go:73-85
Timestamp: 2025-09-09T21:08:03.154Z
Learning: In Go 1.22 and later versions, loop variables are per-iteration (not per-loop), eliminating the classic loop variable capture problem in goroutines. The `tc := tc` shadowing pattern is no longer needed for parallel subtests or other goroutine scenarios when the project uses Go 1.22+.
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-21T06:31:02.454Z
Learning: The gaissmai/bart project uses Go 1.24.0 (as seen in go.mod), so loop variable capture issues that existed in pre-Go 1.22 versions are not relevant. Suggestions to fix loop variable capture in parallel tests should not be made for this project.
Comment on lines
+74
to
+85
| // skip reserved/experimental ranges (e.g., 240.0.0.0/8) | ||
| if pfx.Overlaps(mpp("240.0.0.0/8")) { | ||
| continue | ||
| } | ||
|
|
||
| if _, ok := set[pfx]; !ok { | ||
| set[pfx] = struct{}{} | ||
| pfxs = append(pfxs, pfx) | ||
| } | ||
| } | ||
| return pfxs | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Owner
Author
There was a problem hiding this comment.
Replace mpp calls with netip.MustParsePrefix in internal/nodes/gold_test.go
@coderabbitai mpp is defined in internal/nodes/ tests
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for mpp function definition in internal/nodes package
rg -n --type=go 'func\s+mpp\s*\(' internal/nodes/
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search more broadly for mpp in internal/nodes test files
rg -n --type=go '\bmpp\b' internal/nodes/ | head -20
# Also check if it's defined as a variable
rg -n --type=go 'var\s+mpp|mpp\s*=' internal/nodes/
Length of output: 1668
@gaissmai You're absolutely right, and I apologize for the confusion. I can now confirm that mpp is indeed defined in the internal/nodes test files:
- In
internal/nodes/tests_tmpl.go(line 23):var mpp = netip.MustParsePrefix - In
internal/nodes/nodebasics_test.go(line 48):var mpp = func(s string) netip.Prefix { ... }
The code in internal/nodes/gold_test.go is correct as-is and doesn't need any changes. Thank you for the correction!
🧬 Code Graph Analysis Results
-
Filename: internal/nodes/nodebasics.go
-
Snippet (lines 128-134):
func CmpPrefix(a, b netip.Prefix) int {
if cmpAddr := a.Addr().Compare(b.Addr()); cmpAddr != 0 {
return cmpAddr
}return cmp.Compare(a.Bits(), b.Bits())}
-
-
Filename: internal/nodes/gold_test.go
- Summary:
- mpp(string) is used to construct netip.Prefix values (e.g., "240.0.0.0/8", "2000::/3") within test helpers. Replace each call to mpp with netip.MustParsePrefix, ensuring the same string literals are parsed into netip.Prefix values. The mpp usage appears in the randomRealWorldPrefixes4/prng and randomRealWorldPrefixes6/prng helper functions. If mpp is defined in internal/nodes tests, ensure netip.MustParsePrefix is used consistently in these test builders.
- Summary:
✏️ Learnings added
Learnt from: gaissmai
PR: gaissmai/bart#337
File: internal/nodes/gold_test.go:74-85
Timestamp: 2025-10-12T15:21:07.784Z
Learning: In the internal/nodes package, test files define a helper `mpp` (short for "must parse prefix") that wraps netip.MustParsePrefix. This helper is used throughout the internal/nodes test files to create netip.Prefix values. Do not flag uses of mpp in internal/nodes test files as undefined.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-12T19:34:43.211Z
Learning: Use netip.MustParsePrefix() from the standard library instead of creating custom helper functions for parsing network prefixes in tests. This follows the KISS principle and reduces unnecessary code.
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.
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-23T20:54:23.347Z
Learning: When composing netip prefixes from an Addr in tests/utilities, prefer netip.PrefixFrom(addr, bits) (single return) rather than Addr.Prefix(bits) which returns (Prefix, error) in newer Go versions. This avoids “multiple-value … in single-value context” compile errors.
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.
scripts/generate-table-methods.sh
Outdated
| # build output filename e.g. bartiterators_gen.go | ||
| output_file="${tableType,,}" # lowercase e.g. Fast -> fast | ||
| output_file="${output_file}${template_file/_tmpl/_gen}" # concat with mangled template filename | ||
| output_file="${output_file/common/}" # common -> |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/nodes/barttestsgenerated_test.go(1 hunks)internal/nodes/fasttestsgenerated_test.go(1 hunks)internal/nodes/litetestsgenerated_test.go(1 hunks)internal/nodes/tests_tmpl.go(1 hunks)scripts/generate-node-tests.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/nodes/tests_tmpl.go
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-09-09T21:08:03.154Z
Learnt from: gaissmai
PR: gaissmai/bart#284
File: equal_test.go:73-85
Timestamp: 2025-09-09T21:08:03.154Z
Learning: In Go 1.22 and later versions, loop variables are per-iteration (not per-loop), eliminating the classic loop variable capture problem in goroutines. The `tc := tc` shadowing pattern is no longer needed for parallel subtests or other goroutine scenarios when the project uses Go 1.22+.
Applied to files:
internal/nodes/barttestsgenerated_test.go
📚 Learning: 2025-09-21T06:31:02.454Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-21T06:31:02.454Z
Learning: In Go 1.22 and later, the loop variable capture problem in goroutines has been fixed. Loop variables are now per-iteration instead of per-loop, so the classic `tc := tc` shadowing pattern is no longer needed in parallel subtests or goroutine scenarios when using Go 1.22+.
Applied to files:
internal/nodes/barttestsgenerated_test.go
📚 Learning: 2025-09-21T06:31:02.454Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-21T06:31:02.454Z
Learning: The gaissmai/bart project uses Go 1.24.0 (as seen in go.mod), so loop variable capture issues that existed in pre-Go 1.22 versions are not relevant. Suggestions to fix loop variable capture in parallel tests should not be made for this project.
Applied to files:
internal/nodes/barttestsgenerated_test.go
📚 Learning: 2025-10-06T05:52:05.967Z
Learnt from: gaissmai
PR: gaissmai/bart#333
File: internal/nodes/fastnode.go:333-350
Timestamp: 2025-10-06T05:52:05.967Z
Learning: The gaissmai/bart repository requires Go 1.24.0 (as specified in go.mod). Loop variables in Go 1.22+ are scoped per-iteration by default, so taking the address of loop variables like `&kidAny` is safe and does not require creating a separate copy variable.
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/litetestsgenerated_test.go
📚 Learning: 2025-10-05T20:27:35.772Z
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.
Applied to files:
internal/nodes/barttestsgenerated_test.gointernal/nodes/fasttestsgenerated_test.gointernal/nodes/litetestsgenerated_test.go
📚 Learning: 2025-09-21T22:31:00.914Z
Learnt from: gaissmai
PR: gaissmai/bart#317
File: scripts/geniterators.sh:25-26
Timestamp: 2025-09-21T22:31:00.914Z
Learning: In the gaissmai/bart codebase, the filename generation logic in scripts/geniterators.sh uses bash parameter expansion correctly: ${nodeType/Node/} removes "Node" from nodeType, and ${template_file/_tmpl/_gen} replaces "_tmpl" with "_gen" in the template filename. This produces the correct output filenames like bartnodeiterators_gen.go, fastnodeiterators_gen.go, etc.
Applied to files:
scripts/generate-node-tests.sh
📚 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
📚 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-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/litetestsgenerated_test.go
🧬 Code graph analysis (4)
internal/nodes/barttestsgenerated_test.go (2)
internal/nodes/bartnode.go (1)
BartNode(26-43)internal/nodes/nodebasics.go (1)
StridePath(33-33)
internal/nodes/fasttestsgenerated_test.go (2)
internal/nodes/fastnode.go (1)
FastNode(21-51)internal/nodes/nodebasics.go (1)
StridePath(33-33)
scripts/generate-node-tests.sh (2)
scripts/generate-node-methods.sh (3)
INFO(6-6)WARN(7-7)DIE(8-8)scripts/generate-table-methods.sh (3)
INFO(6-6)WARN(7-7)DIE(8-8)
internal/nodes/litetestsgenerated_test.go (2)
internal/nodes/litenode.go (1)
LiteNode(62-69)internal/nodes/nodebasics.go (1)
StridePath(33-33)
|
Tip For best results, initiate chat on the files or code changes. @gaissmai, acknowledged and understood. You're absolutely correct—the integer range syntax is valid for Go 1.24.0. I've recorded this to avoid similar mistakes in future reviews. Thank you for the correction! 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/nodes/barttestsgenerated_test.go (1)
59-59: Typos inherited from template.The test name "one leave" (line 59) and inline comments "leave in level 1" (lines 148, 160) should use "leaf" instead of "leave". Since this is generated code, fix the typo in
tests_tmpl.goline 81 and regenerate.Also applies to: 148-148, 160-160
🧹 Nitpick comments (2)
internal/nodes/tests_tmpl.go (1)
81-81: Fix typo: "leave" should be "leaf".The test name contains a grammatical error.
Apply this diff to fix the typo in the template (will propagate to generated tests):
- name: "one leave in root node IPv6", + name: "one leaf in root node IPv6",internal/nodes/litetestsgenerated_test.go (1)
59-59: Typos inherited from template.The test name "one leave" (line 59) and inline comments "leave in level 1" (lines 148, 160) should use "leaf" instead of "leave". Since this is generated code, fix the typo in
tests_tmpl.goline 81 and regenerate.Also applies to: 148-148, 160-160
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/nodes/barttestsgenerated_test.go(1 hunks)internal/nodes/fasttestsgenerated_test.go(1 hunks)internal/nodes/litetestsgenerated_test.go(1 hunks)internal/nodes/tests_tmpl.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/nodes/fasttestsgenerated_test.go
🧰 Additional context used
🧠 Learnings (11)
📚 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/tests_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/tests_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/litetestsgenerated_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-05T20:27:35.772Z
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.
Applied to files:
internal/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/litetestsgenerated_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/litetestsgenerated_test.gointernal/nodes/barttestsgenerated_test.go
📚 Learning: 2025-09-09T21:08:03.154Z
Learnt from: gaissmai
PR: gaissmai/bart#284
File: equal_test.go:73-85
Timestamp: 2025-09-09T21:08:03.154Z
Learning: In Go 1.22 and later versions, loop variables are per-iteration (not per-loop), eliminating the classic loop variable capture problem in goroutines. The `tc := tc` shadowing pattern is no longer needed for parallel subtests or other goroutine scenarios when the project uses Go 1.22+.
Applied to files:
internal/nodes/barttestsgenerated_test.go
📚 Learning: 2025-09-21T06:31:02.454Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-21T06:31:02.454Z
Learning: In Go 1.22 and later, the loop variable capture problem in goroutines has been fixed. Loop variables are now per-iteration instead of per-loop, so the classic `tc := tc` shadowing pattern is no longer needed in parallel subtests or goroutine scenarios when using Go 1.22+.
Applied to files:
internal/nodes/barttestsgenerated_test.go
📚 Learning: 2025-09-21T06:31:02.454Z
Learnt from: gaissmai
PR: gaissmai/bart#0
File: :0-0
Timestamp: 2025-09-21T06:31:02.454Z
Learning: The gaissmai/bart project uses Go 1.24.0 (as seen in go.mod), so loop variable capture issues that existed in pre-Go 1.22 versions are not relevant. Suggestions to fix loop variable capture in parallel tests should not be made for this project.
Applied to files:
internal/nodes/barttestsgenerated_test.go
📚 Learning: 2025-10-06T05:52:05.967Z
Learnt from: gaissmai
PR: gaissmai/bart#333
File: internal/nodes/fastnode.go:333-350
Timestamp: 2025-10-06T05:52:05.967Z
Learning: The gaissmai/bart repository requires Go 1.24.0 (as specified in go.mod). Loop variables in Go 1.22+ are scoped per-iteration by default, so taking the address of loop variables like `&kidAny` is safe and does not require creating a separate copy variable.
Applied to files:
internal/nodes/barttestsgenerated_test.go
🧬 Code graph analysis (3)
internal/nodes/tests_tmpl.go (1)
internal/nodes/nodebasics.go (3)
StatsT(51-57)StridePath(33-33)CloneFunc(363-363)
internal/nodes/litetestsgenerated_test.go (2)
internal/nodes/litenode.go (1)
LiteNode(62-69)internal/nodes/nodebasics.go (1)
StridePath(33-33)
internal/nodes/barttestsgenerated_test.go (2)
internal/nodes/bartnode.go (1)
BartNode(26-43)internal/nodes/nodebasics.go (1)
StridePath(33-33)
This was referenced Oct 12, 2025
Merged
Closed
Merged
This was referenced Feb 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests / Chores