-
Notifications
You must be signed in to change notification settings - Fork 203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use unique_ptr for Pimple Transform #1922
Conversation
@@ -80,7 +80,7 @@ class SITKCommon_EXPORT AffineTransform | |||
|
|||
protected: | |||
|
|||
void SetPimpleTransform( PimpleTransformBase *pimpleTransform ) override; | |||
void SetPimpleTransform(std::unique_ptr<PimpleTransformBase> &&pimpleTransform ) override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for asking me to have a look!
The &&
are not really necessary here, you may just pass the unique_ptr
"by value". Example (https://godbolt.org/z/h4cs6fzWo):
void MyFunc(std::unique_ptr<MyObject> myObj)
{
}
int main()
{
MyFunc(std::make_unique<MyObject>());
}
Of course, you can still use &&
, but pass by value is more common, for unique_ptr. My two cents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know that is the conventions. The move
is still required with the following:
int main()
{
auto uptr = std::make_unique<MyObject>();
MyFunc(std::move(uptr));
}
I suppose there is no more cost to moving the unique_ptr object that just copying a ptr.
755aab7
to
f166838
Compare
@N-Dekker After following your suggestion of changing the r-value reference parameter to pass by value There are some compilation errors on linux system ( before others were canceled ): https://open.cdash.org/viewBuildError.php?buildid=8664641
Any suggestions? |
@N-Dekker After following your recommendation of passing std::unique_ptr by value instead of r-value, I are getting errors on most compilers related to the pimple object type being only a forward declaration. Any further suggestions or recommendations? |
Sorry, I did not know about this problem. I've been trying to reproduce the errors at https://godbolt.org/z/cP7Pas5zT but so far, I don't see them there. You see, I just tried:
Anyway, if there is no other solution, just revert my suggestion to pass |
So this case is a bit more interesting. Here is the compilation error reproduced: If the Thank you for your response. |
It does not show up at my side. Can you please select Share >> Short link in the Godbolt site, and share the link? Or otherwise just copy-paste your code? |
Correct link: https://godbolt.org/z/7fMaxG9bd |
I see, you pass the unique_ptr-to-an-incomplete-type forward from Looking at https://www.reddit.com/r/cpp_questions/comments/gpfij5/passing_unique_ptr_by_value_vs_by_reference/ they all say unique_ptr should be passed by value, but it looks like you proved them wrong! I can still ask around to see if your case is indeed an exception to the rule. But in the mean time, do whatever you need to avoid those compiler errors! 👍 |
f166838
to
c379a9d
Compare
Off-topic nit-pick: the C++ idiom I think this is PR about is officially called "Pimpl", either in upper or lower case, but without the letter "e". (See https://en.cppreference.com/w/cpp/language/pimpl or http://wiki.c2.com/?PimplIdiom) So you might still consider replacing any "Pimple" with "Pimpl". |
Remove raw pointer usage for the pimple transform with unique_ptr which enforces the logic of ownership to the heap allocated object.