Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds nil-safety checks across multiple table methods (Overlaps, Union, Equal, Size variants, dump utilities) to prevent panics when invoked on nil receivers. Methods now return safe default values or skip operations gracefully instead of dereferencing nil pointers. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
commonmethods_tmpl.go (2)
443-450:⚠️ Potential issue | 🟠 MajorUnion’s nil‑receiver branch doesn’t update the caller.
Assigning to the receiver variable (
t = o.Clone()) has no effect outside the method. If the intent is fail‑fast, remove this branch; if the intent is nil‑tolerant union, the API must return the new table (or accept **Table).Based on learnings: In the gaissmai/bart codebase, do not suggest nil-receiver guards for methods. The design philosophy is to let programmer errors (like calling methods on nil receivers) cause immediate panics rather than be handled defensively.🔧 Suggested fix (fail‑fast)
func (t *_TABLE_TYPE[V]) Union(o *_TABLE_TYPE[V]) { if o == nil || o == t || (o.size4 == 0 && o.size6 == 0) { return } - if t == nil { - t = o.Clone() - return - } // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commonmethods_tmpl.go` around lines 443 - 450, The method _TABLE_TYPE[V].Union contains a nil-receiver branch that assigns to the receiver (`if t == nil { t = o.Clone(); return }`), which has no effect on the caller; remove that branch so the method no longer attempts to handle a nil receiver (i.e., delete the `if t == nil { ... }` block) and let nil-receiver calls panics per project convention; keep the rest of Union (including the nil/empty-check for o and early returns) untouched.
414-435:⚠️ Potential issue | 🟠 MajorNil‑receiver guards here break the repo’s fail‑fast contract.
Overlaps/Overlaps4/Overlaps6 now quietly return false on nil receivers instead of panicking, which hides programmer errors. Please remove these guards (and apply the same policy to other methods that were changed in this template).
Based on learnings: In the gaissmai/bart codebase, do not suggest nil-receiver guards for methods. The design philosophy is to let programmer errors (like calling methods on nil receivers) cause immediate panics rather than be handled defensively.🔧 Suggested rollback (example for Overlaps*)
func (t *_TABLE_TYPE[V]) Overlaps(o *_TABLE_TYPE[V]) bool { - if t == nil || o == nil { - return false - } return t.Overlaps4(o) || t.Overlaps6(o) } func (t *_TABLE_TYPE[V]) Overlaps4(o *_TABLE_TYPE[V]) bool { - if t == nil || o == nil || t.size4 == 0 || o.size4 == 0 { - return false - } + if t.size4 == 0 || o.size4 == 0 { + return false + } return t.root4.Overlaps(&o.root4, 0) } func (t *_TABLE_TYPE[V]) Overlaps6(o *_TABLE_TYPE[V]) bool { - if t == nil || o == nil || t.size6 == 0 || o.size6 == 0 { - return false - } + if t.size6 == 0 || o.size6 == 0 { + return false + } return t.root6.Overlaps(&o.root6, 0) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commonmethods_tmpl.go` around lines 414 - 435, Remove the nil-receiver guards introduced in _TABLE_TYPE[V].Overlaps, Overlaps4, and Overlaps6 so these methods no longer return false when t or o is nil; instead allow the runtime to panic on nil receiver usage per the repo's fail-fast policy. Specifically, remove checks "if t == nil || o == nil" (and the size checks in Overlaps4/Overlaps6 if they were added as part of the defensive change) and call t.Overlaps4(o) || t.Overlaps6(o), t.root4.Overlaps(&o.root4, 0) and t.root6.Overlaps(&o.root6, 0) directly; apply the same removal of nil-receiver guards to any other methods in this template that were similarly changed.lite.go (1)
404-429:⚠️ Potential issue | 🟠 MajorOverlaps doc says nil‑unsafe but code now guards nil.
This is a direct behavior/doc mismatch and also violates the repo’s fail‑fast policy for nil receivers. Please either remove the nil guards or update the documentation and related tests consistently (preferred: remove guards).
Based on learnings: In the gaissmai/bart codebase, do not suggest nil-receiver guards for methods. The design philosophy is to let programmer errors (like calling methods on nil receivers) cause immediate panics rather than be handled defensively.🔧 Suggested rollback
func (l *Lite) Overlaps(o *Lite) bool { - if l == nil || o == nil { - return false - } return l.liteTable.Overlaps(&o.liteTable) } func (l *Lite) Overlaps4(o *Lite) bool { - if l == nil || o == nil { - return false - } return l.liteTable.Overlaps4(&o.liteTable) } func (l *Lite) Overlaps6(o *Lite) bool { - if l == nil || o == nil { - return false - } return l.liteTable.Overlaps6(&o.liteTable) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lite.go` around lines 404 - 429, Remove the nil-receiver guards from Lite.Overlaps, Lite.Overlaps4, and Lite.Overlaps6 so they match the documentation and the repo's fail-fast policy: delete the "if l == nil || o == nil { return false }" checks and call l.liteTable.Overlaps(&o.liteTable) (and Overlaps4/Overlaps6) directly; update or remove any tests that expect a non-panicking nil-safe behavior to expect a panic on nil receiver instead.litespecial_test.go (1)
57-78:⚠️ Potential issue | 🟠 MajorNil‑receiver noPanic expectations conflict with Lite’s fail‑fast behavior.
These tests now encode nil‑tolerant behavior for Size*/Overlaps*/Equal/dump* on nil receivers, which is inconsistent with the existing Lite policy. Please revert these to mustPanic (or change the API contract globally).
Based on learnings: 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.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@litespecial_test.go` around lines 57 - 78, The tests assert nil-receiver tolerance for Lite methods but Lite intentionally panics on nil receivers; change the expectations for nil-receiver calls to mustPanic instead of noPanic for the Lite-specific methods (Size, Size4, Size6, Overlaps, Overlaps4, Overlaps6, Equal, dump, dumpString) when invoked on a nil tbl1, and keep noPanic only for non-nil invocations (e.g., tbl2.Overlaps(tbl2), tbl1.Overlaps(tbl2), tbl1.Equal(tbl1), tbl2.Equal(tbl2)); locate these calls by the method names (Size, Overlaps*, Equal, dump*, dumpString) and replace the helper from noPanic to mustPanic where the receiver is the nil Lite instance.commontests_tmpl.go (1)
147-168:⚠️ Potential issue | 🟠 MajorNil-receiver noPanic expectations conflict with fail‑fast policy.
The new noPanic expectations for Size*/Overlaps*/Equal/Union on a nil receiver codify behavior that contradicts the repo’s fail‑fast design. These should remain in the mustPanic set unless the project’s policy has changed.
Based on learnings: In the gaissmai/bart codebase, do not suggest nil-receiver guards for methods. The design philosophy is to let programmer errors (like calling methods on nil receivers) cause immediate panics rather than be handled defensively.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commontests_tmpl.go` around lines 147 - 168, The tests add noPanic expectations for methods that should fail fast on a nil receiver; revert those to mustPanic. Replace the noPanic assertions that call methods on the potentially nil table (tbl1) — specifically Size, Size4, Size6, Overlaps(nil), Overlaps4(nil), Overlaps6(nil), Equal(tbl2/ tbl1), Union(tbl2) and UnionPersist(tbl2) when invoked on tbl1 — with mustPanic so nil-receiver calls panic per project policy; leave non-nil-calls (e.g., calls on tbl2 or comparisons between non-nil tables) unchanged.litemethodsgenerated.go (2)
459-468:⚠️ Potential issue | 🟡 Minor
Equal: nil self-comparison returnsfalse— inconsistent with the non-nil self-comparison fast-path.The
o == tshort-circuit that makest.Equal(t)returntruesits after thet == nil || o == nilguard, so whentis nil:var t *liteTable[int] t.Equal(t) // → false (hits t == nil first) t2 := new(liteTable[int]) t2.Equal(t2) // → true (hits o == t fast-path)Moving
o == tbefore the nil checks makes every table equal to itself and preserves all other semantics.🐛 Proposed fix
func (t *liteTable[V]) Equal(o *liteTable[V]) bool { - if t == nil || o == nil || t.size4 != o.size4 || t.size6 != o.size6 { - return false - } if o == t { return true } + if t == nil || o == nil || t.size4 != o.size4 || t.size6 != o.size6 { + return false + } return t.root4.EqualRec(&o.root4) && t.root6.EqualRec(&o.root6) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@litemethodsgenerated.go` around lines 459 - 468, The self-comparison fast-path in liteTable.Equal is after the nil-checks, causing nil receivers to return false for t.Equal(t); move the o == t short-circuit to the top of func (t *liteTable[V]) Equal(o *liteTable[V]) bool so any table (including nil) compares equal to itself, and keep the existing nil checks for the other-case (t == nil || o == nil || t.size4 != o.size4 || t.size6 != o.size6) and the final return using t.root4.EqualRec(&o.root4) && t.root6.EqualRec(&o.root6) unchanged.
382-400:⚠️ Potential issue | 🟠 Major
Unionwith nil receiver creates a discarded clone — dead code / wasted allocation.When
t == nilandois non-nil with data,t = o.Clone()assigns to the local copy of the receiver pointer. Go cannot propagate that assignment back to the caller, so the clone is immediately garbage-collected and the caller's*liteTablestaysnil. The intent appears to be "initialize the nil receiver to a copy ofo", but Go's value-semantics for pointer receivers make this impossible in a void method.The only correct nil-receiver behavior for a void
Unionis a no-op. Thet == nilbranch should simplyreturn.Because this file is generated from
commonmethods_tmpl.go, the same dead-clone exists inbartmethodsgenerated.goandfastmethodsgenerated.go; the fix belongs in the template.🐛 Proposed fix
func (t *liteTable[V]) Union(o *liteTable[V]) { - if o == nil || o == t || (o.size4 == 0 && o.size6 == 0) { + if t == nil || o == nil || o == t || (o.size4 == 0 && o.size6 == 0) { return } - if t == nil { - t = o.Clone() - return - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@litemethodsgenerated.go` around lines 382 - 400, The Union method for *liteTable (function liteTable[V].Union) mistakenly attempts to initialize a nil receiver by assigning t = o.Clone(), which only modifies the local receiver copy and wastes an allocation; replace that branch with a simple return to make nil-receiver behavior a no-op. Update the template in commonmethods_tmpl.go (which generates liteTable.Union, bartTable.Union, fastTable.Union variants) to remove the t == nil { t = o.Clone(); return } logic and instead early-return when t == nil, and regenerate the derived files (bartmethodsgenerated.go and fastmethodsgenerated.go) so the dead-clone is removed across all generated Union methods.
🧹 Nitpick comments (1)
litemethodsgenerated.go (1)
407-410:UnionPersistwith nil receiver returnsnil— consider whethero.Clone()is the right semantic.When
t == nilbutohas routes, the current guard returnsnil(the nilt). The union of "nothing" and "something" is arguably "something", so the caller may expect a clone ofo. However, since the existing tests only assert no-panic and this aligns with treating nil as "uninitialised / not comparable", this is a design-level question rather than a bug.If the intent is "nil receiver is a valid empty table", returning
o.Clone()whent == nil && ois non-empty would be semantically cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@litemethodsgenerated.go` around lines 407 - 410, The current guard in UnionPersist returns the receiver when t == nil, which makes nil ∪ non-empty return nil; change the early-return logic in liteTable[V].UnionPersist so that if t == nil and o != nil and (o.size4 != 0 || o.size6 != 0) the method returns o.Clone() instead, while keeping the existing returns for cases where both are nil or o is empty (and still returning t for o == t). Adjust the conditional so it distinguishes the t==nil && o non-empty case and calls o.Clone() to represent the union correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fastmethodsgenerated.go`:
- Around line 459-462: The Equal method currently guards against nil receivers
which violates the codebase contract that calling Fast.Equal on a nil receiver
should panic; remove the nil checks so the method remains fail‑fast: in function
Fast.Equal (symbols t, o, size4, size6) delete the t == nil || o == nil checks
and only compare sizes (e.g. if t.size4 != o.size4 || t.size6 != o.size6 {
return false }) so a nil t will naturally panic when accessed.
In `@lite.go`:
- Around line 218-257: These new nil checks make Lite methods inconsistent with
the existing fail-fast behavior; remove the nil-receiver guards in Size, Size4,
Size6, dump, and dumpString so they call l.liteTable.* directly (e.g., Size ->
return l.liteTable.Size()) and will panic on nil receivers like
String/MarshalText do; ensure references to liteTable remain unchanged so
behavior matches Table/Fat and existing tests that expect panic on nil *Lite.
---
Outside diff comments:
In `@commonmethods_tmpl.go`:
- Around line 443-450: The method _TABLE_TYPE[V].Union contains a nil-receiver
branch that assigns to the receiver (`if t == nil { t = o.Clone(); return }`),
which has no effect on the caller; remove that branch so the method no longer
attempts to handle a nil receiver (i.e., delete the `if t == nil { ... }` block)
and let nil-receiver calls panics per project convention; keep the rest of Union
(including the nil/empty-check for o and early returns) untouched.
- Around line 414-435: Remove the nil-receiver guards introduced in
_TABLE_TYPE[V].Overlaps, Overlaps4, and Overlaps6 so these methods no longer
return false when t or o is nil; instead allow the runtime to panic on nil
receiver usage per the repo's fail-fast policy. Specifically, remove checks "if
t == nil || o == nil" (and the size checks in Overlaps4/Overlaps6 if they were
added as part of the defensive change) and call t.Overlaps4(o) ||
t.Overlaps6(o), t.root4.Overlaps(&o.root4, 0) and t.root6.Overlaps(&o.root6, 0)
directly; apply the same removal of nil-receiver guards to any other methods in
this template that were similarly changed.
In `@commontests_tmpl.go`:
- Around line 147-168: The tests add noPanic expectations for methods that
should fail fast on a nil receiver; revert those to mustPanic. Replace the
noPanic assertions that call methods on the potentially nil table (tbl1) —
specifically Size, Size4, Size6, Overlaps(nil), Overlaps4(nil), Overlaps6(nil),
Equal(tbl2/ tbl1), Union(tbl2) and UnionPersist(tbl2) when invoked on tbl1 —
with mustPanic so nil-receiver calls panic per project policy; leave
non-nil-calls (e.g., calls on tbl2 or comparisons between non-nil tables)
unchanged.
In `@lite.go`:
- Around line 404-429: Remove the nil-receiver guards from Lite.Overlaps,
Lite.Overlaps4, and Lite.Overlaps6 so they match the documentation and the
repo's fail-fast policy: delete the "if l == nil || o == nil { return false }"
checks and call l.liteTable.Overlaps(&o.liteTable) (and Overlaps4/Overlaps6)
directly; update or remove any tests that expect a non-panicking nil-safe
behavior to expect a panic on nil receiver instead.
In `@litemethodsgenerated.go`:
- Around line 459-468: The self-comparison fast-path in liteTable.Equal is after
the nil-checks, causing nil receivers to return false for t.Equal(t); move the o
== t short-circuit to the top of func (t *liteTable[V]) Equal(o *liteTable[V])
bool so any table (including nil) compares equal to itself, and keep the
existing nil checks for the other-case (t == nil || o == nil || t.size4 !=
o.size4 || t.size6 != o.size6) and the final return using
t.root4.EqualRec(&o.root4) && t.root6.EqualRec(&o.root6) unchanged.
- Around line 382-400: The Union method for *liteTable (function
liteTable[V].Union) mistakenly attempts to initialize a nil receiver by
assigning t = o.Clone(), which only modifies the local receiver copy and wastes
an allocation; replace that branch with a simple return to make nil-receiver
behavior a no-op. Update the template in commonmethods_tmpl.go (which generates
liteTable.Union, bartTable.Union, fastTable.Union variants) to remove the t ==
nil { t = o.Clone(); return } logic and instead early-return when t == nil, and
regenerate the derived files (bartmethodsgenerated.go and
fastmethodsgenerated.go) so the dead-clone is removed across all generated Union
methods.
In `@litespecial_test.go`:
- Around line 57-78: The tests assert nil-receiver tolerance for Lite methods
but Lite intentionally panics on nil receivers; change the expectations for
nil-receiver calls to mustPanic instead of noPanic for the Lite-specific methods
(Size, Size4, Size6, Overlaps, Overlaps4, Overlaps6, Equal, dump, dumpString)
when invoked on a nil tbl1, and keep noPanic only for non-nil invocations (e.g.,
tbl2.Overlaps(tbl2), tbl1.Overlaps(tbl2), tbl1.Equal(tbl1), tbl2.Equal(tbl2));
locate these calls by the method names (Size, Overlaps*, Equal, dump*,
dumpString) and replace the helper from noPanic to mustPanic where the receiver
is the nil Lite instance.
---
Duplicate comments:
In `@bartmethodsgenerated.go`:
- Around line 382-389: The nil-receiver branch in Table[V].Union that does "if t
== nil { t = o.Clone(); return }" is ineffective because assigning to the
receiver local doesn't affect the caller; remove this nil-guard and any
defensive handling for nil receivers in Union, relying on the project convention
to let nil-receiver calls panic, and regenerate the code; target the Union
method on type Table[V] and any related template that emits the nil-receiver
guard (and it may reference Clone) to ensure the guard is not reintroduced when
regenerating.
- Around line 408-523: Remove the duplicated nil-receiver guards from the
generated methods (e.g., the UnionPersist/UnionRecPersist caller wrapper,
Table.Equal, Table.Size, Table.Size4, Table.Size6) in bartmethodsgenerated.go
and instead implement the nil-handling in the shared template
commonmethods_tmpl.go, then re-run the generator to regenerate
bartmethodsgenerated.go; specifically, delete the if t == nil ... checks from
the listed methods (Table.Equal, Table.Size*, and the UnionPersist entry point)
so the generated file matches the template-driven behavior and relies on the
common template for nil-guard policy.
- Around line 353-374: Update the template commonmethods_tmpl.go to stop adding
nil-receiver guards: remove checks that test the receiver (t == nil) in the
generated Overlaps, Overlaps4, and Overlaps6 methods so the receiver behaves as
before (allowing a nil receiver to panic) while leaving any legitimate argument
checks (e.g., for o == nil or size checks) as appropriate, then regenerate
bartmethodsgenerated.go so the Overlaps/Overlaps4/Overlaps6 implementations no
longer include t == nil guards.
In `@commonmethods_tmpl.go`:
- Around line 469-586: Remove the defensive nil-receiver checks added in the
template: revert the early nil checks in the union method (the guard that starts
with "if t == nil || o == nil ..." around UnionPersist/UnionRecPersist usage),
the Equal method (remove the "if t == nil || o == nil || t.size4 != o.size4 ..."
guard and the short-circuit), and the Size/Size4/Size6 methods (remove the if t
== nil returns). Restore the original behavior so these methods
(UnionPersist/UnionRecPersist-related logic, _TABLE_TYPE[V].Equal,
_TABLE_TYPE[V].Size, Size4, Size6) do not swallow nil receiver panics; update
the template accordingly and regenerate the code.
In `@fastmethodsgenerated.go`:
- Around line 382-389: The Union method contains a defensive nil-receiver branch
("if t == nil { t = o.Clone(); return }") which is ineffective and should be
removed at the template level; update the commonmethods_tmpl.go template to
eliminate any nil-receiver guard for the Fast Union implementation (the code
that emits func (t *Fast[V]) Union(...)), regenerate fastmethodsgenerated.go so
the generated Union method no longer tries to handle a nil receiver and will
instead allow a panic if called on nil.
- Around line 505-523: Remove the defensive nil-receiver guards from the
generated methods by editing the template in commonmethods_tmpl.go: eliminate
the "if t == nil { return 0 }" checks used to render Fast[V].Size4 and
Fast[V].Size6 (and the analogous Size method) so the generated methods access
fields directly (e.g., return t.size4, return t.size6, and return t.size4 +
t.size6); then regenerate fastmethodsgenerated.go so the output reflects the
template change.
- Around line 353-374: The generated methods added defensive nil-receiver checks
which violate the project's policy; update the template in commonmethods_tmpl.go
to stop emitting "t == nil" guards and then regenerate fastmethodsgenerated.go
so Overlaps, Overlaps4, and Overlaps6 no longer check the receiver for nil
(remove expressions like "t == nil ||" from those functions), leaving other
legitimate checks (e.g., o == nil or size==0) as appropriate.
In `@fasttestsgenerated_test.go`:
- Around line 70-71: The generated test lines calling mustPanic(t,
"OverlapsPrefix", func() { tbl1.OverlapsPrefix(pfx4) }) and mustPanic(t,
"OverlapsPrefix", func() { tbl1.OverlapsPrefix(pfx6) }) correctly mirror the
template and require no change; leave the calls to mustPanic and the
OverlapsPrefix invocations on tbl1 with pfx4 and pfx6 as-is.
- Around line 77-99: Update the test template commontests_tmpl.go so it does not
generate duplicate "nil-receiver noPanic" expectations: remove or change the
generated noPanic invocations that call methods on a nil receiver (the lines
that call noPanic(t, "Size"/"Size4"/"Size6", func() { tbl1.Size() }) and similar
for Overlaps/Overlaps4/Overlaps6/Equal/Union and their tbl1/tbl2 nil
permutations) so the tests no longer assert "no panic" for nil receivers;
instead let those cases be omitted or assert panic according to project policy,
then regenerate fasttestsgenerated_test.go so tbl1/tbl2 calls reflect the
corrected expectations.
In `@lite.go`:
- Around line 438-441: The method Equal currently checks both the receiver and
the argument for nil, which conflicts with the codebase policy of allowing
nil-receiver panics; update the Equal method to remove the receiver nil check
(remove "l == nil" from the if condition) but keep the nil check for the
parameter "o" and the size mismatch checks, then return
l.liteTable.Equal(&o.liteTable) as before so a nil receiver will panic while a
nil argument still returns false; refer to the Equal method and fields size4,
size6, and liteTable to locate the change.
---
Nitpick comments:
In `@litemethodsgenerated.go`:
- Around line 407-410: The current guard in UnionPersist returns the receiver
when t == nil, which makes nil ∪ non-empty return nil; change the early-return
logic in liteTable[V].UnionPersist so that if t == nil and o != nil and (o.size4
!= 0 || o.size6 != 0) the method returns o.Clone() instead, while keeping the
existing returns for cases where both are nil or o is empty (and still returning
t for o == t). Adjust the conditional so it distinguishes the t==nil && o
non-empty case and calls o.Clone() to represent the union correctly.
Comment on lines
459
to
462
| func (t *Fast[V]) Equal(o *Fast[V]) bool { | ||
| if o == nil || t.size4 != o.size4 || t.size6 != o.size6 { | ||
| if t == nil || o == nil || t.size4 != o.size4 || t.size6 != o.size6 { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Fast.Equal must remain fail‑fast on nil receivers.
Fast.Equal is intentionally not nil‑safe in this codebase. The new nil guards change that contract and should be removed.
🔧 Suggested rollback
func (t *Fast[V]) Equal(o *Fast[V]) bool {
- if t == nil || o == nil || t.size4 != o.size4 || t.size6 != o.size6 {
+ if t.size4 != o.size4 || t.size6 != o.size6 {
return false
}📝 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
| func (t *Fast[V]) Equal(o *Fast[V]) bool { | |
| if o == nil || t.size4 != o.size4 || t.size6 != o.size6 { | |
| if t == nil || o == nil || t.size4 != o.size4 || t.size6 != o.size6 { | |
| return false | |
| } | |
| func (t *Fast[V]) Equal(o *Fast[V]) bool { | |
| if t.size4 != o.size4 || t.size6 != o.size6 { | |
| return false | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fastmethodsgenerated.go` around lines 459 - 462, The Equal method currently
guards against nil receivers which violates the codebase contract that calling
Fast.Equal on a nil receiver should panic; remove the nil checks so the method
remains fail‑fast: in function Fast.Equal (symbols t, o, size4, size6) delete
the t == nil || o == nil checks and only compare sizes (e.g. if t.size4 !=
o.size4 || t.size6 != o.size6 { return false }) so a nil t will naturally panic
when accessed.
Comment on lines
+218
to
+257
| // Size returns the prefix count. | ||
| func (l *Lite) Size() int { | ||
| if l == nil { | ||
| return 0 | ||
| } | ||
| return l.liteTable.Size() | ||
| } | ||
|
|
||
| // Size4 returns the IPv4 prefix count. | ||
| func (l *Lite) Size4() int { | ||
| if l == nil { | ||
| return 0 | ||
| } | ||
| return l.liteTable.Size4() | ||
| } | ||
|
|
||
| // Size6 returns the IPv6 prefix count. | ||
| func (l *Lite) Size6() int { | ||
| if l == nil { | ||
| return 0 | ||
| } | ||
| return l.liteTable.Size6() | ||
| } | ||
|
|
||
| // dump the table structure and all the nodes to w. | ||
| func (l *Lite) dump(w io.Writer) { | ||
| if l == nil { | ||
| return | ||
| } | ||
| l.liteTable.dump(w) | ||
| } | ||
|
|
||
| // dumpString is just a wrapper for dump. | ||
| func (l *Lite) dumpString() string { | ||
| if l == nil { | ||
| return "" | ||
| } | ||
| return l.liteTable.dumpString() | ||
| } | ||
|
|
There was a problem hiding this comment.
New Size/dump methods are nil‑safe, which makes Lite’s nil behavior inconsistent.
Lite methods historically panic on nil receivers (e.g., String/MarshalText); these new guards introduce a split behavior that will confuse callers. If the policy remains fail‑fast, drop the nil checks; otherwise document and update all Lite methods/tests for consistent nil‑tolerance.
🔧 Suggested rollback (fail‑fast)
func (l *Lite) Size() int {
- if l == nil {
- return 0
- }
return l.liteTable.Size()
}
func (l *Lite) Size4() int {
- if l == nil {
- return 0
- }
return l.liteTable.Size4()
}
func (l *Lite) Size6() int {
- if l == nil {
- return 0
- }
return l.liteTable.Size6()
}
func (l *Lite) dump(w io.Writer) {
- if l == nil {
- return
- }
l.liteTable.dump(w)
}
func (l *Lite) dumpString() string {
- if l == nil {
- return ""
- }
return l.liteTable.dumpString()
}📝 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
| // Size returns the prefix count. | |
| func (l *Lite) Size() int { | |
| if l == nil { | |
| return 0 | |
| } | |
| return l.liteTable.Size() | |
| } | |
| // Size4 returns the IPv4 prefix count. | |
| func (l *Lite) Size4() int { | |
| if l == nil { | |
| return 0 | |
| } | |
| return l.liteTable.Size4() | |
| } | |
| // Size6 returns the IPv6 prefix count. | |
| func (l *Lite) Size6() int { | |
| if l == nil { | |
| return 0 | |
| } | |
| return l.liteTable.Size6() | |
| } | |
| // dump the table structure and all the nodes to w. | |
| func (l *Lite) dump(w io.Writer) { | |
| if l == nil { | |
| return | |
| } | |
| l.liteTable.dump(w) | |
| } | |
| // dumpString is just a wrapper for dump. | |
| func (l *Lite) dumpString() string { | |
| if l == nil { | |
| return "" | |
| } | |
| return l.liteTable.dumpString() | |
| } | |
| // Size returns the prefix count. | |
| func (l *Lite) Size() int { | |
| return l.liteTable.Size() | |
| } | |
| // Size4 returns the IPv4 prefix count. | |
| func (l *Lite) Size4() int { | |
| return l.liteTable.Size4() | |
| } | |
| // Size6 returns the IPv6 prefix count. | |
| func (l *Lite) Size6() int { | |
| return l.liteTable.Size6() | |
| } | |
| // dump the table structure and all the nodes to w. | |
| func (l *Lite) dump(w io.Writer) { | |
| l.liteTable.dump(w) | |
| } | |
| // dumpString is just a wrapper for dump. | |
| func (l *Lite) dumpString() string { | |
| return l.liteTable.dumpString() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lite.go` around lines 218 - 257, These new nil checks make Lite methods
inconsistent with the existing fail-fast behavior; remove the nil-receiver
guards in Size, Size4, Size6, dump, and dumpString so they call l.liteTable.*
directly (e.g., Size -> return l.liteTable.Size()) and will panic on nil
receivers like String/MarshalText do; ensure references to liteTable remain
unchanged so behavior matches Table/Fat and existing tests that expect panic on
nil *Lite.
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
1 participant
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
Release Notes
New Features
Size(),Size4(),Size6()and debugging utilitiesdump(),dumpString()on Lite tables.Bug Fixes