search

ros / urdf

Repository for URDF parsing code
63 stars 41 forks source link

Changed tinyxml2 parameter to be const reference #14

Closed sloretz closed 6 years ago

sloretz commented 6 years ago

This changes the initXml API to take a const reference to tinyxml2 objects. Changing to const because it can, and to a reference to eliminate the need to check for a null pointer.

sloretz commented 6 years ago

@clalancette if this looks OK to you then I'll need to update ros/kdl_parser#4 to pass a reference instead of a pointer.

clalancette commented 6 years ago

@sloretz So, after a bunch of digging into C++ (thanks to @mjcarroll, @wjwwood , and @scpeters ), the reason the test failed is that the compiler actually coerced the tinyxml2::XMLElement* returned by RootElement to an XMLDocument(bool, tinyxml2::Whitespace) due to XMLElement having an XMLDocument conversion constructor and due to XMLDocument(bool, tinyxml2::Whitespace) not being explicit. I'll push a fix shortly.

However, this does bring up a question, which is what users are expecting. While I agree that a const ref is safer in general, it seems like that is not how tinyxml2 operates, so switching it here may make consumers of this API have to do more work. I'm not sure if that is a good idea, and I'm inclined to leave this as-is (i.e. pointers). @sloretz , what do you think?

sloretz commented 6 years ago

@clalancette Matching tinyxml2's API sounds reasonable to me. Nicely done tracking down that converting constructor.

How about making the parameter a pointer to a const object?

clalancette commented 6 years ago

How about making the parameter a pointer to a const object?

That makes sense; we promise not to change what we are given. I'll make that change later today.

clalancette commented 6 years ago

That makes sense; we promise not to change what we are given. I'll make that change later today.

This is now done in #15. I'm going to close this one out in favor of that.