openmc icon indicating copy to clipboard operation
openmc copied to clipboard

Cleanup of C++ core codebase

Open paulromano opened this issue 6 years ago • 2 comments

After #1171 is merged, we no longer have any Fortran code in OpenMC. However, there are still some things that need cleaning up on the C++ side. I wanted to open this issue to keep track of them:

  • [ ] Fix off-by-one indexing. Much of the C API still expects indices based on 1-indexing instead of 0-indexing. As a result, there is a whole bunch of -1 and +1s scattered through the code. We should consistently use 0-indexing now that there is no Fortran.
  • [x] Replace xyz and uvw with the Position and Direction classes (mostly as part of the Particle class)
  • [x] The Particle::initialize method should probably be part of a constructor?
  • [x] See if we can get rid of MAXCOORD and MAX_SECONDARY?
  • [ ] Use the GSL library for spans (to replace pointer/length pairs in interfaces) and asserts
  • [ ] Use trailing underscore for class data members consistently (especially Particle)
  • [ ] Use enum class where it makes sense
  • [x] Use unique_ptr instead of raw pointers in global vectors

If any of you guys have other things in mind that I'm missing, feel free to add to the list.

paulromano avatar Feb 22 '19 21:02 paulromano

Following up on this... there are lots of places where we have "off by one" comments on surface logic. The thing about surfaces is that we want "-1" and "1" to point to the 0th surface but in a different sense. I think as is it's good and we should perhaps remove the TODOs and replace them with a brief explanatory comment why this approach is used. Although, if you'd like to move to an approach which doesn't rely on subtracting one from the absolute value of the surface index, I'd totally understand that.

gridley avatar Mar 31 '21 00:03 gridley

@gridley I think the line of thought is that we wouldn't use a plain int to represent a surface half-space in Cell::region_ and Particle::surface_. Instead, we could have a half-space class with surface() and side() methods, the first of which returns the surface index (0-based). This prevents the -1 and 1 issue you're describing. Also, @smharper has suggested that the underlying data (surface and side) could be encoded within 32/64 bits, whichever we decide to use.

paulromano avatar Apr 07 '21 13:04 paulromano