Skip to content
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

world.h: remove initXml function, tinyxml include #39

Merged
merged 2 commits into from
Oct 10, 2017

Conversation

scpeters
Copy link
Contributor

@scpeters scpeters commented Aug 3, 2017

The urdf::World::initXml function is never implemented
and causes an unnecessary tinyxml dependency.
So remove that function and the tinyxml include.

The urdf::World class uses a pointer to a
tinyxml symbol in the initXml function,
but we don't want to export that dependency,
so just forward declare the symbol since it's a pointer.
@jslee02
Copy link
Contributor

jslee02 commented Sep 18, 2017

Can we merge this? This PR is blocking ros/urdfdom#99 (build error).

@traversaro
Copy link
Contributor

Just to understand: we are keeping the void initXml(TiXmlElement* config); instead of removing it to preserve the ABI, right?

@jslee02
Copy link
Contributor

jslee02 commented Sep 18, 2017

Just to understand: we are keeping the void initXml(TiXmlElement* config); instead of removing it to preserve the ABI, right?

I believe so.

Also, for the consistency with urdfdom, I would like to suggest either switching to TinyXML2 for urdfdom_headers or removing initXml because ros/urdfdom#99 will break the API anyway. I'm inclined to removing initXml since it's not implemented.

@clalancette
Copy link
Contributor

I did a little bit of code archeology here, and as far as I can tell, initXml has never been implemented for the World class. Thus, there is no way any downstream users can be dependent on it; they would always fail to compile. Additionally, back in 2012, in commit ea2982a, all of the other initXml methods on all of the other classes were removed, presumably for the same reasons. Finally, if you look at the World class closely, it is clearly meant to be used as a header-only class. Thus, I would strongly suggest that we just kill off the initXml method here, and remove the tinyxml dependency from this header file. I've done that in 468ad1d ; @scpeters feel free to cherry-pick that here.

This allows us to remove tinyxml.h from the header file.

Signed-off-by: Chris Lalancette <[email protected]>
@scpeters scpeters merged commit 995aa1e into ros:master Oct 10, 2017
@scpeters scpeters deleted the remove_initXml branch October 10, 2017 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants