Selecting shared_ptr by SIRF engines. #62
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…s (save for trivial methods)
… DataContainer.m for argument check
Closed
|
@casperdcl do you have any idea why github claims that this PR contains so many commits? This PR is based on master, which was merged to devel recently, from which this branch was created. I guess the git merge won't be a problem, but I'd like to understand this a bit more. (not sure if this is why it claims there will be conflicts) |
|
@evgueni-ovtchinnikov I had a brief look at this. It seems a bit of a hack to me. I guess it works so I'm ok with merging it, but I'm generally quite warry of solving this stuff with I'm not 100% sure, but I guess what you'd need is to have an extra template arg for your |
|
Yes, files object_handle.inl are identical – I should have had just one of them in src/common, but I just do not know how to edit CMakeLists so that it is found.
Extra template arg would just be too much editing, with namespace it is just two lines per file.
From: Kris Thielemans [mailto:notifications@github.com]
Sent: 27 September 2017 14:03
To: CCPPETMR/SIRF
Cc: Ovtchinnikov, Evgueni (STFC,RAL,SC); Mention
Subject: Re: [CCPPETMR/SIRF] Selecting shared_ptr by SIRF engines. (#62)
@evgueni-ovtchinnikov<https://github.com/evgueni-ovtchinnikov> I had a brief look at this. It seems a bit of a hack to me. I guess it works so I'm ok with merging it, but I'm generally quite warry of solving this stuff with #define. Also, it seems we have twice object_handle.inl (identical?) .
I'm not 100% sure, but I guess what you'd need is to have an extra template arg for your ObjectHandle class (and others?)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#62 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AOWfVAQYdnm3XFqfdyr6DTSncRy8s1N4ks5smkdTgaJpZM4PljW0>.
|
|
Could it be because I branched it from devel?
From: Kris Thielemans [mailto:notifications@github.com]
Sent: 27 September 2017 13:59
To: CCPPETMR/SIRF
Cc: Ovtchinnikov, Evgueni (STFC,RAL,SC); Author
Subject: Re: [CCPPETMR/SIRF] Selecting shared_ptr by SIRF engines. (#62)
@casperdcl<https://github.com/casperdcl> do you have any idea why github claims that this PR contains so many commits? This PR is based on master, which was merged to devel recently, from which this branch was created. I guess the git merge won't be a problem, but I'd like to understand this a bit more.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#62 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AOWfVO03LAGXNjtNc46f2vY897QFrXnDks5smkaBgaJpZM4PljW0>.
|
|
FYI, modifying CMake files:
I wouldn't do this right now, as we want to avoid the can you merge with master locally first. looks like you'll have conflicts. |
|
Merged locally, no conflicts.
From: Kris Thielemans [mailto:notifications@github.com]
Sent: 27 September 2017 15:14
To: CCPPETMR/SIRF
Cc: Ovtchinnikov, Evgueni (STFC,RAL,SC); Mention
Subject: Re: [CCPPETMR/SIRF] Selecting shared_ptr by SIRF engines. (#62)
FYI, modifying CMake files:
1. create src/common/include/SIRF/common or something
2. in xSTIR/CMakeLists.txt do something like
include_directories(${PROJECT_SOURCE_DIR}/src/common/include)
1. in your file
#include "SIRF/common/object_handle.inl"
I wouldn't do this right now, as we want to avoid the #define trick later (it's not good programming style, and I'm worried that we'll have a conflict at some point).
can you merge with master locally first. looks like you'll have conflicts.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#62 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AOWfVDqJN12ZNEwWTUHE6h9f4xbT1CUfks5smlgYgaJpZM4PljW0>.
|
|
ok. please push |
|
Done
From: Kris Thielemans [mailto:notifications@github.com]
Sent: 27 September 2017 15:42
To: CCPPETMR/SIRF
Cc: Ovtchinnikov, Evgueni (STFC,RAL,SC); Mention
Subject: Re: [CCPPETMR/SIRF] Selecting shared_ptr by SIRF engines. (#62)
ok. please push
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#62 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AOWfVKau1i5nZrgomx852Pq6EpHa-2Zdks5sml7BgaJpZM4PljW0>.
|
|
Merged manually |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Switched to the use of std::shared_ptr in xGadgetron and stir::shared_ptr in xSTIR. Generally, different SIRF engines can now use different shared_ptr (as specified in *_shared_ptr,h).