Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CellId.AllNeighbors has documented but unchecked precondition #104

Open
ymofen opened this issue Dec 15, 2023 · 3 comments
Open

CellId.AllNeighbors has documented but unchecked precondition #104

ymofen opened this issue Dec 15, 2023 · 3 comments

Comments

@ymofen
Copy link

ymofen commented Dec 15, 2023

ll1 := s2.LatLngFromDegrees(30.755702, 114.127656)
c1 := s2.CellIDFromLatLng(ll1)
neighbors := c1.AllNeighbors(8)

image

@jmr
Copy link
Collaborator

jmr commented Apr 1, 2025

Can you put this in the form of a test case we can add to latlng_test.go?

@jmr
Copy link
Collaborator

jmr commented Apr 3, 2025

c1 is level 30. You can't get level 8 neighbors from that.

// This requires level >= ci.Level().

You can try c1 := s2.CellIDFromLatLng(ll1).Parent(8).

@rsned Apparently go has no assert / DCHECK. Should this also return an ok bool? There must be many more examples of unchecked preconditions waiting to bite someone and waste time.

@jmr jmr changed the title AllNeighbors has a bug CellId.AllNeighbors has documented but unchecked precondition Apr 3, 2025
@rsned
Copy link
Collaborator

rsned commented Apr 3, 2025

It is possible to set up a dcheck equivalent using go build tags. e.g. #141. There are a couple places where I noted the c++ checks something here, but not too many of the DCHECKs are directly referenced in the code.

jmr added a commit to jmr/geo that referenced this issue Apr 9, 2025
Return nil if level is too low (cells are too large and might not be
neighbors) or higher than MaxLevel (no such cells exist).  Weakens
precondition of function.

golang#104 complained that AllNeighbors
was returning the wrong results, but the level precondition was being
violated.  Make this more obvious and less confusing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants