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

myers/middle_snake: correct safety doc #11

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

jonathantanmy
Copy link
Contributor

Hello, one last thing that my colleague noticed. This PR is assuming that bounds_check is correct - I haven't scrutinized the algorithm enough to determine what the exact bounds are.

Again, feel free to change to meet your project's style. (This passes cargo fmt, but I don't know how long you prefer your documentation lines to be.)

Commit message follows:

In the documentation for MiddleSnakeSearch::new, the safety constraints described on the data pointer do not exactly match how the class uses it. It is stored as the kvec field, and looking at the write_xpos_at_diagonal, x_pos_at_diagonal, and pos_at_diagonal functions (a search of kvec in the source code revealing that all access to kvec go through one of these functions):

  • writes could happen, not only reads
  • the range of access is as described in the bounds_check function

Therefore, update the documentation accordingly.

In the documentation for MiddleSnakeSearch::new, the safety constraints
described on the `data` pointer do not exactly match how the class
uses it. It is stored as the `kvec` field, and looking at the
`write_xpos_at_diagonal`, `x_pos_at_diagonal`, and `pos_at_diagonal`
functions (a search of `kvec` in the source code revealing that all
access to `kvec` go through one of these functions):

- writes could happen, not only reads
- the range of access is as described in the bounds_check function

Therefore, update the documentation accordingly.
@pascalkuthe
Copy link
Owner

pascalkuthe commented Jul 26, 2024

I think I wrote this safety comment very early when I started working on this and then didn't update it as the code evolved (as it's just an internal function). I did a patch release for the last fix since that could feasibly have negative effects but since this is just internal documentation and doesn't affect users I will merge this without cutting a release

@pascalkuthe pascalkuthe merged commit 6fc0dc1 into pascalkuthe:master Jul 26, 2024
5 checks passed
@jonathantanmy
Copy link
Contributor Author

Makes sense. Thanks for taking a look.

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