Skip to content
This repository has been archived by the owner on Aug 3, 2020. It is now read-only.

Add missing namespaces #165

Closed
wants to merge 1 commit into from
Closed

Conversation

jspricke
Copy link
Member

@jspricke jspricke commented Nov 4, 2016

No description provided.

@wjwwood
Copy link
Member

wjwwood commented Nov 5, 2016

Can you provide some context as to why this is needed? Should the *SharedPtr typedefs be inside a namespace (urdf) as well? @rhaschke

@jspricke
Copy link
Member Author

jspricke commented Nov 5, 2016

ModelInterface is in the urdf namespace, so we have to add it, otherwise gcc complains. Adding the namespace to the *SharedPtr definitely makes sense. Actually why not move them into urdf/include/urdf/model.h?

@rhaschke
Copy link
Contributor

rhaschke commented Nov 5, 2016

I don't see a reason, why the explicit namespace is needed. All these types are declared within a namespace urdf. Some older gcc might have issues with that.
Moving the declaration to urdf/include/urdf/model.h doesn't make sense, as ModelInterfaceis a type defined by urdfdom_headers, not the urdfpackage. However, I suggested to have this declaration in urdf_headersinstead: ros/urdfdom_headers#33. Unfortunately, nobody commented on this idea yet.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be on the safe side, I suggest to explicitly state ::urdf::ModelInterface.

@jspricke
Copy link
Member Author

jspricke commented Nov 5, 2016

urdf namespace ends in line 72 whereas these defines are in 78ff. gcc 6.2.0 (current Debian unstable) complained.

@rhaschke
Copy link
Contributor

rhaschke commented Nov 6, 2016

urdf namespace ends in line 72 whereas these defines are in 78ff. gcc 6.2.0 (current Debian unstable) complained.

Oh, indeed. I missed that. In this case, please extend the namespace to lines 78ff. as the SharedPtr types should reside in that namespace too as suggested in #165 (comment).

@jspricke
Copy link
Member Author

jspricke commented Nov 6, 2016

Tested and works fine with new added namespace

@jspricke
Copy link
Member Author

jspricke commented Nov 6, 2016

I've inegrated this into #144.

@jspricke jspricke closed this Nov 6, 2016
@jspricke jspricke deleted the add_namespace branch November 6, 2016 15:05
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.

3 participants