search

rustwasm / book

The Rust and WebAssembly Book
https://rustwasm.github.io/docs/book/
MIT License
1.75k stars 211 forks source link

Index bounds check #98

Open smcmurray opened 6 years ago

smcmurray commented 6 years ago

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? 🤔

smcmurray commented 6 years ago

Added issue #99 for this, as directed.

SomeoneToIgnore commented 5 years ago

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.

fitzgen commented 5 years ago

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!

SomeoneToIgnore commented 5 years ago

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