pcd_grabber fix destruction #1127
Conversation
soyersoyer
force-pushed
the
pcdgrabber_fix
branch
from
3e8dec7 to
5840c17
Compare
| @@ -387,7 +387,6 @@ pcl::PCDGrabberBase::PCDGrabberBase (const std::vector<std::string>& pcd_files, | |||
| /////////////////////////////////////////////////////////////////////////////////////////// | |||
| pcl::PCDGrabberBase::~PCDGrabberBase () throw () | |||
| { | |||
| stop (); | |||
| delete impl_; | |||
There was a problem hiding this comment.
We might need to be more careful here because the copy constructor and assignment operator of PCDGrabberBase both copy this raw pointer. Would wrapping it into a shared pointer be a good idea?
There was a problem hiding this comment.
Unfortunately it is not a good idea. The trigger function calls the outside class' (PCDGrabberBase) publish function. https://github.com/soyersoyer/pcl/blob/pcdgrabber_fix/io/src/pcd_grabber.cpp#L291
After a copy it will call the copied PCDGrabberBase's publish function, not the new.
I think the PCDGrabberBase is not a good "value class" candidate (it is polymorphic), so the copy constructor and assignment should be private/deleted. If the copy is necessary, it is achievable with a virtual clone function.
There was a problem hiding this comment.
Okay. It's not clear why they were made public in the first place. Anyways, that's a separate issue. Could you put the opening brace in dtor on a separate line so I can merge?
There was a problem hiding this comment.
Sorry, I've forgotten it. Fixed :-)
PCDGrabberImpl::trigger () use the PCDGrabber<PointT>'s publish () virtual function, so the ~PCDGrabber<PointT> should stop the PCDGrabberBase
soyersoyer
force-pushed
the
pcdgrabber_fix
branch
from
5840c17 to
ef4175a
Compare
PCDGrabberImpl::trigger () use the PCDGrabber's publish () virtual function, so the ~PCDGrabber should stop the PCDGrabberBase