[CCPPETMR/SIRF] Clone as sptr (#277)
Richard Brown
We need this so that we can create copies of derived image classes, stored as the base ImageData class.
We'll need this for registration etc., as the output will be a copy of one of the inputs (which will be stored as the abstract base class, ImageData).
You can view, comment on, or merge this pull request online at:
https://github.com/CCPPETMR/SIRF/pull/277
Commit Summary
- Clone as sptr
File Changes
- M src/common/include/sirf/common/ImageData.h (2)
- M src/xGadgetron/cGadgetron/gadgetron_data_containers.cpp (10)
- M src/xGadgetron/cGadgetron/include/sirf/cGadgetron/gadgetron_data_containers.h (7)
- M src/xSTIR/cSTIR/include/sirf/cSTIR/stir_data_containers.h (6)
Patch Links:
—
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
@KrisThielemans commented on this pull request.
I think most of these could use make_shared. However, shouldn't we be using @evgueni-ovtchinnikov's strategy of having a virtual function that creates a ptr (using covariant return types), and just making the shared_ptr at base class level?
Also, I think we have a naming problem. I cannot remember how we called the recent functions in that discussion. We do have gadgetron::shared_ptr<MRAcquisitionData> clone(). All of these should have the same name. for the same concept.
I'd be fine with clone return the bare pointer, and clone_as_sptr as a shared_ptr, or clone returning the shared_ptr and clone_as_ptr, or another sensible convention, but mixing things up is bad.
Richard Brown
I not overly fussed about which name we choose, but clone_as_sptr seems to make the most sense to me.
If you agree, I'll rename gadgetron::shared_ptr<MRAcquisitionData> clone() as gadgetron::shared_ptr<MRAcquisitionData> clone_as_sptr() and then they would all be consistent.
I think most of these could use
make_shared.
We could, I can do that if you prefer (seems the same to me since they both call the constructor once).
However, shouldn't we be using @evgueni-ovtchinnikov's strategy of having a virtual function that creates a ptr (using covariant return types), and just making the shared_ptr at base class level?
I'd rather not do that, since we'd be creating double the number of methods, and I'd err away from using raw pointers anyway.
Thoughts?
Kris Thielemans
This seems to overlap with #254. Doesn't that give you what you need? You might need of course some ugly casting. But that can be resolved implementing a clone_as_sptr in terms of new_data_container_sptr (doing the casting there). No other code needed.
In fact, somewhat surprisingly, this trick can be used with the same function name (using "hiding").
I've created a separate issue for the longer term design, see #278. I guess you don't need this PR if you can use new_data_container_sptr. (Note however that there's an extra constructor for GadgetronImagesVector in this PR.)
(By the way, I think make_shared is a tiny bit more conservative in memory usage than shared_ptr(new T(...)) but it's probably the least of our worries).
Richard Brown
new_data_container_sptr returns a blank copy of that image type. In hindsight, I don't need new_data_container_sptr, but I do need a copy method.
So, could we:
If that's ok, I'll update this PR as per #278 so that clone_as_sptr is renamed as copy and returns a std::unique_ptr. I'll also use make_shared.
Richard Brown
Implemented as per #278. Travis fine (as much as master).
Kris Thielemans
Merged #277 into master.
Kris Thielemans
@rijobro, please create an issue to "lift" this up to DataContainer