remove ~RobotModelLoader

moveit / moveit_ros

THIS REPO HAS MOVED TO https://github.com/ros-planning/moveit

70 stars 118 forks source link

remove ~RobotModelLoader #705

Closed v4hn closed 8 years ago

v4hn commented 8 years ago

The destructor only resetted shared_ptr of the class. This automatically happens after the destructor finished anyway and the explicit resets disturb the default destructor order of the data structures.

Additionally this has the same effect as #702 and for some reason works around #592 over here. This is not meant as a fix for #592, but it resolves the problem for the moment without adding new unnecessary code. The real bug, as @rhaschke pointed out, lies in internal design faults of pluginlib and class_loader and has to be resolved there.

davetcoleman commented 8 years ago

+1 makes sense to me, those classes already clear themselves

alainsanguinetti commented 8 years ago

@v4hn I tested this and it works for me in my cpp code!

rhaschke commented 8 years ago

Checking with AddressSanitizer, this PR of course doesn't resolve #592, although - by chance - it doesn't crashes the program in normal operation mode anymore. Of course, if there is no need for a specific destruction order of members, the destructor could be removed. +1

rhaschke commented 8 years ago

Only noticed now: Probably this PR destroys ABI compatibility, because it now falls back to the default destructor?

v4hn commented 8 years ago

I have no idea why I failed to notice that before merging, but it is ABI breaking. Therefore I reverted the change in indigo-devel and applied it to jade-devel only.

alainsanguinetti commented 8 years ago

What if you simply remove the .reset() from the destructor ?

v4hn commented 8 years ago

Sure, but

functionally it does not make a difference, so why bother for a two step solution, when this is already fixed in jade-devel?
there are quite a number of such resets to be found throughout MoveIt and I don't see a reason to add another commit for one of them, when someone (me included) might sit down in the future and get rid of all of them systematically? :)