Skip to content
This repository has been archived by the owner on Nov 13, 2017. It is now read-only.

remove ~RobotModelLoader #705

Merged

Conversation

v4hn
Copy link
Contributor

@v4hn v4hn commented Jul 6, 2016

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.

The destructor only resetted shared_ptr of the class.
This automatically happens after the destructor anyway
and it does disturb the default destructor order of the
data structures.
@davetcoleman
Copy link
Member

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

@alainsanguinetti
Copy link
Contributor

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

@rhaschke
Copy link
Contributor

rhaschke commented Jul 7, 2016

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

@v4hn v4hn merged commit 93d76a8 into moveit:indigo-devel Jul 7, 2016
v4hn added a commit that referenced this pull request Jul 7, 2016
This reverts commit 93d76a8.

This is not ABI-compatible, so remove it from indigo-devel.
@rhaschke
Copy link
Contributor

rhaschke commented Jul 7, 2016

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

v4hn added a commit that referenced this pull request Jul 7, 2016
The destructor only resetted shared_ptr of the class.
This automatically happens after the destructor anyway
and it does disturb the default destructor order of the
data structures.
@v4hn
Copy link
Contributor Author

v4hn commented Jul 7, 2016 via email

@alainsanguinetti
Copy link
Contributor

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

@v4hn
Copy link
Contributor Author

v4hn commented Jul 7, 2016

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? :)

alainsanguinetti pushed a commit to alainsanguinetti/moveit_ros that referenced this pull request Jul 8, 2016
The destructor only resetted shared_ptr of the class.
This automatically happens after the destructor anyway
and it does disturb the default destructor order of the
data structures.
alainsanguinetti pushed a commit to alainsanguinetti/moveit_ros that referenced this pull request Jul 8, 2016
This reverts commit 93d76a8.

This is not ABI-compatible, so remove it from indigo-devel.
otamachan pushed a commit to otamachan/moveit_ros that referenced this pull request Oct 22, 2017
The destructor only resetted shared_ptr of the class.
This automatically happens after the destructor anyway
and it does disturb the default destructor order of the
data structures.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants