lumol-org / lumol

Universal extensible molecular simulation engine
http://lumol.org/
BSD 3-Clause "New" or "Revised" License
191 stars 18 forks source link

Mark `types::Array` indexing as unsafe #136

Open Luthaf opened 7 years ago

Luthaf commented 7 years ago

Follow-up of https://github.com/lumol-org/lumol/pull/130#issuecomment-290916546.

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 commented 7 years ago

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

antoinewdg commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

antoinewdg commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Ok, I agree with you =)

antoinewdg commented 7 years ago

Yaaayyy !

Luthaf commented 7 years ago

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