book icon indicating copy to clipboard operation
book copied to clipboard

Index bounds check

Open smcmurray opened this issue 5 years ago • 4 comments

get_index should still vet its arguments.

✋ A similar PR may already be submitted! Please search 🔎 among the open pull requests before creating one.

✋ If there isn't a similar PR, is there an open issue associated with what this PR is trying to solve? If not, let's create one before opening a PR. ✨

Now that you've checked, it's time to create your PR. 📝 Thanks for submitting! 🙏

For more information, see the contributing guide. 👫

Summary

Explain the motivation for making this change. What existing problem does the pull request solve? 🤔

  • [x] 👯 This PR is not a duplicate
  • [x] 📬 This PR addresses an open issue
  • [x] ✅ This PR has passed CI

smcmurray avatar Sep 22 '18 22:09 smcmurray

Added issue #99 for this, as directed.

smcmurray avatar Sep 22 '18 23:09 smcmurray

I like the way it's implemented in the pull request (using % instead of debug_assert), allowing the javascript code to receive the index for any row and column pair specified without any panics.

Consider an excersize from https://rustwasm.github.io/book/game-of-life/interactivity.html#exercises where a pulsar should be added: if a user clicks close enough to the edge of the universe, the part of the figure to be added will appear on the other side of the universe (since it's periodic). If we go with the % approach, then implementing this behavior is rather trivial – we simply add some offsets to the clicked point row and column and call get_index for those. With debug_assert! approach we will be forced to write extra boilerplate to ensure that those offset values are in the universe range instead.

If you're fine with the approach from the PR, I can create a PR into the https://github.com/rustwasm/wasm_game_of_life repo.

SomeoneToIgnore avatar Nov 23 '18 15:11 SomeoneToIgnore

If we go with the % approach, then implementing this behavior is rather trivial – we simply add some offsets to the clicked point row and column and call get_index for those.

This should be handled further up the stack, before we ever call Universe::get_index.

I can create a PR into the https://github.com/rustwasm/wasm_game_of_life repo.

That would be very appreciated!

fitzgen avatar Nov 26 '18 17:11 fitzgen

Makes sense, here is the PR: https://github.com/rustwasm/wasm_game_of_life/pull/38

SomeoneToIgnore avatar Nov 26 '18 22:11 SomeoneToIgnore