GalSim-developers/GalSim — pull request #183 by rmjarvis (2012-06-06 15:28:35+00:00)
Title: Branch #107 Added shared_ptr wrapping to a lot of current code that used bare pointers.

There had been quite a few places in the code where we had been passing around bare pointers to things that required the calling function to be responsible for deleting. Similarly, there were places where a reference was passed into an object, and the caller was responsible for keeping the original. Both situations are improved with the use of shared_ptr's, so that's what this branch implemented. (It covers both issues #107 and #176.)

- SBProfile had the biggest change from adding shared_ptr. I basically made SBProfile just have a shared_ptr to its implementation. Then things that used to return a `SBProfile*` can now return an `SBProfile` by value (or one of the derived classes, like SBDistort), which works fine, because the shared_ptr behind the scenes keeps track of all the objects that are pointing to the same data, and only deletes it once the last object goes out of scope.
- To make this work intuitively, it was helpful to make all SBProfile objects immutable. So once they are constructed, there are no (public) methods to change them.
- The biggest thing that needed to change to make that work was `setFlux`. Now, rather than changing the flux internally, `setFlux` returns a new object that wraps around the original with a flux scaling. This is the same way that `shear`, `rotate` and `shift` had been working.
- I also added a new `scaleFlux` method. Since the implementation is now to scale the existing flux value, it seemed worth having a way for the user to be able to specify that factor directly, rather than doing `getFlux` to see what the current flux is, which the implementation would call again to remove that factor.
- SBAdd and SBConvolve can no longer be `add`ed to. You now need to construct them from a complete list of components. (This is how we usually did do it, so it wasn't a problem to remove this functionality.)
- SBInterpolatedImage had a number of mutating methods that allowed it to store multiple images. There was then a `setWeights` method that let you re-weight those images and use the new weighted sum as the image to be interpolating. This was efficient because the fourier transforms of the component images were stored so they didn't have to be recomputed every time you changed the weights. I separated out this functionality into a new class `MultipleImageHelper` (which I would be fine changing the name of if someone has a better idea). So now you can create a MultipleImageHelper object with a list of images. This stores the images and their fourier transforms. Then you can create different SBInterpolatedImages from that and a list of weights, which uses the stored data to calculate the new weighted sum, but doesn't calculate new fourier transforms for everything.
- I also added the k-space Interpolant for SBInterpolatedImage as an optional argument on the constructor. The constructor used to just use a default value, and the user could specify something else with `setKInterpolant`. Now, this would need to be specified at construction if you want something other than the default.
- I removed all the duplicate functions (and uses thereof), since they are now unnecessary. This led to a significant speed up in MultiObjectDemo script 3, since the duplications of SBInterpolatedImage were pretty non-trivial.
- I removed the "OLD_WAY" for SBInterpolatedImage's xValue and kValue. I think these are now very old ways, since they don't use XTable and KTable. Since I don't think they are pedagogical for understanding the new way that does use XTable and KTable, I didn't think it was worth preserving them in the code anymore.
- I changed how the various Deviate classes (and CCDNoise) store their random number generator, so we now don't require keeping the UniformDeviate around after we pass it into a GaussianDeviate (say). Now all the Deviates are part of a class hierarchy, and the BaseDeviate class is the one that stores the RNG. It stores this in a shared_ptr, so if you construct any kind of Deviate by passing it another one (of any kind, not just Uniform anymore), the two will share the same RNG. But now you don't have to create them that way. You can also create any kind of deviate with either just its own parameters (in which case it will initialize the RNG seed with the time) or with an explicit seed. These were the ways you used to be able to construct a UniformDeviate, so I just added that functionality to everything.
- I switch the KTable and XTable `transform` methods that used to return bare pointers to the other type to returning shared_ptrs. I don't think there were any places where we forgot to delete the returned value, but it just seemed better coding practice to use a smart pointer here.
- I also encapsulated the FFTW memory allocation/deallocation in a new class FFTW_Array. Again, I don't think the old code was buggy with respect to this, but it's a bit better coding style, IMO.
- InterpolantXY used to take a reference to the 1-d Interpolant that it would use, and the caller was responsible for keeping it alive until the InterpolantXY went out of scope. Now it passes in a shared_ptr to the 1-d Interpolant. So it will be deleted whenever that last thing that uses it goes out of scope.
- The Laguerre stuff was already using a shared_ptr concept, but Gary rolled his own version of reference counting and such. I switched that to use a boost::shared_ptr instead. Along the way, I'm pretty sure I fixed a minor bug where the order and the vector could become out of sync with each other (in resize).

Note that the new Impl classes aren't indented properly at the moment. Doing so would mess up git merges, since essentially most of SBProfile.h would look like it was being changed, just because 4 spaces are added to the beginning of the lines. So we should wait to do this until there are no (or very few) outstanding issues being worked on in SBProfile. At the same time, I'd like to split up SBProfile.cpp into several smaller files, since it is getting very large. All of this is maybe half and hour of work, but it would massively disrupt git's ability to merge, so I didn't do any of it now.