Address panic-ing quadtree.Matching(…) method when finding no closest node #73
Conversation
This file contains 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
…ching this test case panics because in quadtree/quadtree.go does not check if the `findVisitor.closest` value is nil before returning `v.closest.Value` at the end of the method. a fix here is not straight forward because the method signature does not return `error`. It's possible that we could add a `type orb.NilPointer` [ ugh naming ] that satisfies the orb.Poitner interface but could be returned without breaking existing clients
just return a private, nilPointer struct that implements orb.Pointer by
returning an orb.Point{} directly. This change is technically breaking
in that it no longer reliably panics if users were relying on that
behavior. It is the simplest change I could think of that didn't change
the API.
Alternatively, we can introduce new FindV2 and MatchingV2 methods that
return (orb.Pointer, error) that do not panic that can survive until a
minor orb version bump with breaking API.
willsalz
changed the title
willsalz
commented
Sep 29, 2021
quadtree/quadtree.go
Outdated
| @@ -181,6 +181,14 @@ func (q *Quadtree) Find(p orb.Point) orb.Pointer { | |||
| return q.Matching(p, nil) | |||
| } | |||
|
|
|||
| type nilPointer struct{} | |||
There was a problem hiding this comment.
what an unfortunate name; consider emptyPointer?
paulmach
reviewed
Oct 12, 2021
quadtree/quadtree.go
Outdated
| @@ -204,6 +212,9 @@ func (q *Quadtree) Matching(p orb.Point, f FilterFunc) orb.Pointer { | |||
| q.bound.Min[1], q.bound.Max[1], | |||
| ) | |||
|
|
|||
| if v.closest == nil { | |||
| return nilPointer{} | |||
There was a problem hiding this comment.
why can't this just return nil? The caller would have the do the if results != nil check, but at this point the code is panicing.
paulmach
reviewed
Oct 12, 2021
| @@ -136,6 +136,12 @@ func TestQuadtreeMatching(t *testing.T) { | |||
| point: orb.Point{0.1, 0.1}, | |||
| expected: orb.Point{1, 1}, | |||
| }, | |||
| { | |||
| name: "match none filter", | |||
There was a problem hiding this comment.
This test will need to be updated. Maybe change the expected to a pointer expected *orb.Point and handle the case below.
|
Thank you for taking the time to make this change. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
a fix here is not straight forward because the method signature does not
return
error. It's possible that we could add atype orb.NilPointer[ ugh naming ] that satisfies the orb.Poitner interface but could be
returned without breaking existing clients