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

getIndexFromPosition() should not return index larger than bufferSize #41

Merged
merged 2 commits into from
Feb 22, 2016

Conversation

czalidis
Copy link
Contributor

In my first commit I added a test case that tests the return value of getIndexFromPosition() when a position right on the edge is given. Turns out that the function returns true and the returned index is invalid (index is equal to bufferSize). I noticed that checkIfPositionWithinMap() accepts positions that are exactly on the edge of the map but when the index is generated there is no check to validate that the index is still in range.

My solution was to modify checkIfPositionWithinMap() and not accept positions on the edge of the map. It seems that this change doesn't affect anything else.

Another solution (if it is desired to accept edge positions) would be to make sure that the returned index will be in-bounds, in similar fashion as the limitPositionToRange() does. In this case, the test that I added in my first commit should pass.

Since I am not sure about the desired behavior of checkIfPositionWithinMap() regarding edge cases, let me know if something else is indeed affected by this change.

@pfankhauser
Copy link
Member

I don't think there's an expected behavior for these edge cases so I would go ahead with your proposed solution. Thanks!

pfankhauser added a commit that referenced this pull request Feb 22, 2016
getIndexFromPosition() should not return index larger than bufferSize
@pfankhauser pfankhauser merged commit ceaa54d into ANYbotics:master Feb 22, 2016
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

Successfully merging this pull request may close these issues.

2 participants