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

Mark types::Array indexing as unsafe #136

Open
Luthaf opened this issue Apr 4, 2017 · 10 comments
Open

Mark types::Array indexing as unsafe #136

Luthaf opened this issue Apr 4, 2017 · 10 comments

Comments

@Luthaf
Copy link
Member

Luthaf commented Apr 4, 2017

Follow-up of #130 (comment).

We have three propositions:

  1. Rename Array2 to UnsafeArray2 and Array3 to UnsafeArray3, but keep the indexing 'safe'.
  2. Remove the unchecked access with Index, and use an unsafe at method
  3. Only implement Index<Unchecked> with struct Unchecked(usize)
@Luthaf
Copy link
Member Author

Luthaf commented Apr 4, 2017

I like the third proposition: it make things explicit, while maintaining readability of the code.

@antoinewdg
Copy link
Contributor

I do not see how 3 is more readable than 2. Adding an extra Unchecked(i,j) in every call is a bit much, while using the unsafe at would only require one unsafe at the beginning of the line.

I know I'm being stubborn with this one, I just really do not like hiding unsafety. Plus we can keep the current Index (adding bound checks), which is still the one that should be used in the majority of cases (i.e. as long as we are not too deep in a nested loop).

@Luthaf
Copy link
Member Author

Luthaf commented Apr 4, 2017

I do not see how 3 is more readable than 2.

Using the 'trick' use types::Unchecked as U; and then self.rho[U(i, j, k)].

@antoinewdg
Copy link
Contributor

antoinewdg commented Apr 4, 2017

self.rho[U(i, j, k)] vs self.at(i, j, k) is not an obvious win, even accounting for the extra unsafe...

@antoinewdg
Copy link
Contributor

I guess the way I see it is "if the Rust team decided that checking bounds only on debug is unsafe, I'll just follow along".

@Luthaf
Copy link
Member Author

Luthaf commented Apr 4, 2017

I we can get to a point where not every access is annotated with unsafe while retaining the performance, I think I can live with the unsafe Array::at(usize, usize) -> &T. But there is sill the issue that we need both Array::at(usize, usize) and Array::at_mut(usize, usize), while indexing uses the same syntax.

@antoinewdg
Copy link
Contributor

That's right.
However in this solution, at_mut will only be used in performance critical code. If possible in the near future, performance critical code will be parallelized, and at_mut can not be used in parallel loops so at_mut won't show up that much :)

@Luthaf
Copy link
Member Author

Luthaf commented Apr 8, 2017

Ok, I agree with you =)

@antoinewdg
Copy link
Contributor

Yaaayyy !

@Luthaf
Copy link
Member Author

Luthaf commented Oct 28, 2017

Just removing the unsafe version did not worked out, so we might want to go the at, at_mut way =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants