-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pointed out some things to fix in here. Most important looks to be the changes to the APIs; we'll have to tick-tock those.
@@ -0,0 +1,43 @@ | |||
/********************************************************************* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename this file to xml_helpers.hpp
, please.
#ifndef ROS2_MASTER_XML_HELPERS_HPP | ||
#define ROS2_MASTER_XML_HELPERS_HPP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#ifndef ROS2_MASTER_XML_HELPERS_HPP | |
#define ROS2_MASTER_XML_HELPERS_HPP | |
#ifndef URDF_PARSER__XML_HELPERS_HPP | |
#define URDF_PARSER__XML_HELPERS_HPP |
using TiXmlDocument = tinyxml2::XMLDocument; | ||
using TiXmlElement = tinyxml2::XMLElement; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really rather not pretend that tinyxml2 APIs are the same as the tinyxml ones; this is changing the API below, but in a sneaky way. Let's remove this, deprecate the tinyxml APIs, and also add in the tinyxml2 APIs.
dynamics_xml->SetAttribute("damping", urdf_export_helpers::values2str(jd.damping) ); | ||
dynamics_xml->SetAttribute("friction", urdf_export_helpers::values2str(jd.friction) ); | ||
xml->LinkEndChild(dynamics_xml); | ||
auto * dynamics_xml = xmlAppendChild(xml, "dynamics"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really prefer not to use auto
unless it is blindingly obvious from this line of code what the type is. It doesn't seem to be the case here, so I'd much prefer using tinyxml2::XMLElement*
here. The same goes for most uses of auto
below.
xml->LinkEndChild(limit_xml); | ||
auto * limit_xml = xmlAppendChild(xml, "limit"); | ||
limit_xml->SetAttribute("effort", jl.effort); | ||
limit_xml->SetAttribute("velocity", jl.velocity ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: no space before the closing parentheses. There are more instances of this below.
@clalancette, feel free to take this over. |
Fix ros#137