[CCPPETMR/SIRF] boost->std::shared_ptr (#61)

0 views
Skip to first unread message

Casper da Costa-Luis

unread,
Sep 24, 2017, 5:44:31 AM9/24/17
to CCPPETMR/SIRF, Subscribed

Doesn't compile as STIR still seems to be using boost::shared_ptr

You can view, comment on, or merge this pull request online at:

  https://github.com/CCPPETMR/SIRF/pull/61

Commit Summary

  • boost->std::shared_ptr

File Changes

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

unread,
Sep 24, 2017, 12:05:21 PM9/24/17
to CCPPETMR/SIRF, Subscribed

I guess this is a straightforward replacement of boost::shared_ptr with std::shared_ptr? This is fine for me but does imply we have to switch to STIR master now (as the version we used at 0.9 still used boost::shared_ptr).

However, logically speaking, what pointers used by STIR and SIRF should be independent of each other. I wonder if the STIR objects shouldn't simply use stir::shared_ptr (which will be compatible with any version). This is something that needs @evgueni-ovtchinnikov though.

Casper da Costa-Luis

unread,
Sep 24, 2017, 12:16:36 PM9/24/17
to CCPPETMR/SIRF, Subscribed

yes, and yes I agree if there's a stir::shared_ptr

Kris Thielemans

unread,
Sep 24, 2017, 12:39:15 PM9/24/17
to CCPPETMR/SIRF, Subscribed

yes. stir::shared_ptr will actually be an alias for either boost::shared_ptr or std::shared_ptr (via using).

evgueni-ovtchinnikov

unread,
Sep 25, 2017, 6:55:52 AM9/25/17
to CCPPETMR/SIRF, Subscribed
If STIR objects in SIRF would use stir::shared_ptr, then so would have to iUtilities objects and functions, and hence our Gadgetron and other prospective engines’ interfaces too.

I suggest we introduce and use sirf::shared_ptr instead.

Kris, how do you switch between boost and std when building STIR?

From: Kris Thielemans [mailto:notifi...@github.com]
Sent: 24 September 2017 17:05
To: CCPPETMR/SIRF
Cc: Ovtchinnikov, Evgueni (STFC,RAL,SC); Mention
Subject: Re: [CCPPETMR/SIRF] boost->std::shared_ptr (#61)


I guess this is a straightforward replacement of boost::shared_ptr with std::shared_ptr? This is fine for me but does imply we have to switch to STIR master now (as the version we used at 0.9 still used boost::shared_ptr).

However, logically speaking, what pointers used by STIR and SIRF should be independent of each other. I wonder if the STIR objects shouldn't simply use stir::shared_ptr (which will be compatible with any version). This is something that needs @evgueni-ovtchinnikov<https://github.com/evgueni-ovtchinnikov> though.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://github.com/CCPPETMR/SIRF/pull/61#issuecomment-331719669>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AOWfVE-kq74FWr7Ld3bJG5fbFrTdOMb5ks5sln2-gaJpZM4Ph1bX>.

Kris Thielemans

unread,
Sep 26, 2017, 6:21:23 AM9/26/17
to CCPPETMR/SIRF, Subscribed

SIRF requires C++-11, so there is no reason to introduce sirf::shared_ptr. SIRF needs to stick to std::shared_ptr.

Are you ever returning STIR shared_ptrs directly in the MATLAB/Python interface? as long as they are used only internally in your C++ classes, there should be no problem to have those as different types of shared_ptr as far as I can see.

evgueni-ovtchinnikov

unread,
Sep 26, 2017, 7:23:35 AM9/26/17
to CCPPETMR/SIRF, Subscribed
Matlab and Python pass around pointers to DataHandle objects, which wrap shared_pointers to STIR/Gadgetron/SIRF objects. Using different shared pointers would require different DataHandle objects then.

Indeed, if we stick to C++-11 from now on, we just need to switch from boost to std, which is trivial.

However, if we ever come across a PET or MR engine that we would want to add to SIRF, it might happen that it uses boost::shared_ptr . ☹

From: Kris Thielemans [mailto:notifi...@github.com]
Sent: 26 September 2017 11:21
To: CCPPETMR/SIRF
Cc: Ovtchinnikov, Evgueni (STFC,RAL,SC); Mention
Subject: Re: [CCPPETMR/SIRF] boost->std::shared_ptr (#61)


SIRF requires C++-11, so there is no reason to introduce sirf::shared_ptr. SIRF needs to stick to std::shared_ptr.

Are you ever returning STIR shared_ptrs directly in the MATLAB/Python interface? as long as they are used only internally in your C++ classes, there should be no problem to have those as different types of shared_ptr as far as I can see.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://github.com/CCPPETMR/SIRF/pull/61#issuecomment-332154563>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AOWfVO5aYXLtttJe6FgjQOVYIYZc0pxMks5smNAbgaJpZM4Ph1bX>.

evgueni-ovtchinnikov

unread,
Sep 26, 2017, 11:21:07 AM9/26/17
to CCPPETMR/SIRF, Subscribed
I implemented the use of independent (i.e. potentially different) shared_ptr by each SIRF engine. xSTIR now uses stir::shared_ptr, and for xGadgetron I tested both boost::shared_ptr and std::shared_ptr.

So should be ok now with both old and new STIR (cannot check with the latter yet as SuperBuild failed on VM), and adding engines that use boost::shared_ptr should not be a problem.

The new stuff is on SIRF branch sirf_sptr.

—

evgueni-ovtchinnikov

unread,
Sep 27, 2017, 6:37:53 AM9/27/17
to CCPPETMR/SIRF, Subscribed

Superseded by #62.

evgueni-ovtchinnikov

unread,
Sep 27, 2017, 6:37:55 AM9/27/17
to CCPPETMR/SIRF, Subscribed

Closed #61.

Reply all
Reply to author
Forward
0 new messages