[CCPPETMR/SIRF] clone in C++ (#278)

[Kris Thielemans]

We have clone in Python/MATLAB classes, but a few differently called functions in C++. DataContainer* DataContainer::new_data_container, gadgetron::shared_ptr<MRAcquisitionData> clone(), and some suggestions in #277 (close_as_sptr) and #254 (new_data_container_sptr). There's few decisions to be made here.

Naming

I think ideally our C++ functions have the same name as the Python/MATLAB ones (doesn't have to be, we could always drop _sptr in Python/MATLAB, as we do in STIR).

Design

Return value (exposure of bare pointers)

SIRF C++ does currently create bare pointers in a few places, but that's dangerous for memory leaks (it's almost impossible to avoid them once an exception is thrown for instance). We should try and minimise this. Good C++ design would return a std::unique_ptr from a clone-type function (it indicates to the user that this is a new object, implying deep copy etc). std::unique_ptr is convertible to std::shared_ptr. I guess we could have a clone-type that does return a pointer to an existing object, but I find that quite dangerous (as I mentioned in #243).

Covariant return type

It would be desirable to use covariant return types. Sadly, in C++ this works for references and bare pointers, but not with smart pointers. See this Fluent C++ blog for an interesting post on covariant return with shared_ptr. Using the heavily templated mechanism suggested over there is probably overkill for us, but some ideas are useful.

For instance

class DataContainer
{
  std::unique_ptr<DataContainer> clone() const; // use clone_impl
private:
  virtual DataContainer* clone_impl() const = 0;
};
class ImageData
{
  std::unique_ptr<ImageData> clone() const;  // use DataContainer::clone() probably
};
class PETImageData
{
  std::unique_ptr<PETImageData> clone() const; // use ImageData::clone() probably
};
class STIRImageData
{
  std::unique_ptr<STIRImageData> clone() const; // use clone_impl I guess (doesn't matter too much)
private:
  virtual STIRImageData* clone_impl() const; // actually implement it
};

Note that I was a bit surprised that DataContainer::clone_impl can apparently be private.

@evgueni-ovtchinnikov, do you need non-private access to the bare pointer functions?

The interesting thing is that if we ever get round to using SWIG, the above design will make sure that Python/MATLAB also have the same data-type as the object it's called from.

Ideas/objections?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

[Kris Thielemans]

I found a few more: gadgetron::shared_ptr<ISMRMRDImageData> ISMRMRDImageData::new_images_container, gadgetron::shared_ptr<ISMRMRDImageData> ISMRMRDImageData::clone(...), gadgetron::shared_ptr<GadgetronImageData> GadgetronImagesVector::new_images_container, gadgetron::shared_ptr<GadgetronImageData> GadgetronImagesVector::clone(...), STIRImageData* STIRImageData::same_image_data(), stir::shared_ptr<STIRImageData> STIRImageData::new_image_data(), DataContainer* STIRImageData::new_data_container()

I'm hoping we can most of these the same (with an exception for those that return a bare ptr)

[Richard Brown]

I think I agree with all this. We should definitely minimise the use of bare pointers, so I have no problem with clone returning a unique_ptr instead of a bare one.

As an initial step, could we omit the clone method from DataContainer, and only put it in ImageData downwards?

[Richard Brown]

We need to check which of these are the same concept and give them all the same name (preferably with virtual methods).

[Richard Brown]

make as many of these methods (e.g., same_*_data) protected/private as possible.

[evgueni-ovtchinnikov]

removed all new_data_container() methods and all same_*_data() except 4 that are necessary for the storage scheme management

all clone() methods return std::unique_ptr

[evgueni-ovtchinnikov]

Closed #278.

[Kris Thielemans]

@rijobro, can you check if this follows the clone scheme in your PR?

[Richard Brown]

Done.