Tinyxml2 support #9
Conversation
|
Looks good to me. I guess this makes #4 obsolete then. |
|
Adding new dependencies needs to be reflected in the package.xml (even if the tinyxml2 module is provided by cmake-modules it doesnt provide the dependency) Building on top of the original contribution will allow the original contributor to be credited. Testing deprecated features will generate deprecated warnings so the test compile arguments should be updated accordingly |
Oh, good call, I missed that bit. Will fix.
True, but I wanted to restructure the whole thing in a pretty significant way. I think what I'll do here is modify the last commit to make @rojkov the author, since most of the code is his.
Sure, I'll add |
clalancette
force-pushed
the
tinyxml2
branch
from
df0d604
to
e0e6866
Compare
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Also add in some tests, and add in a deprecation warning for the TinyXML versions of the APIs. Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com> Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
clalancette
force-pushed
the
tinyxml2
branch
from
e0e6866
to
c8e4fcd
Compare
|
@sloretz Gentle ping for a review here. I'd like to get this into Melodic, as this will be holding up doing a similar change in i.e. kdl_parser. |
| @@ -55,9 +58,13 @@ class Model : public ModelInterface | |||
| { | |||
| public: | |||
| /// \brief Load Model from TiXMLElement | |||
| URDF_EXPORT bool initXml(TiXmlElement * xml); | |||
| URDF_EXPORT URDF_DEPRECATED("TinyXML API is deprecated, use the TinyXML2 version instead") bool initXml(TiXmlElement * xml); | |||
There was a problem hiding this comment.
Could this break someone downstream using -Werror? If so maybe the target branch should be melodic.
There was a problem hiding this comment.
That's a good point. The downside to targeting a melodic branch is that we'll probably end up supporting 3 branches: kinetic-devel (with only tinyxml APIs), melodic-devel (with tinyxml and tinyxml2 APIs and the deprecation warning), and n-turtle-devel (with only tinyxml2 APIs). But that is more conservative, and ensures we don't break kinetic, so I think I'll go ahead and do that.
Thanks for the review!
|
Now that the TinyXML interface is deprecated, can you guys mention this in the migration guide for Melodic and maybe point to an example diff for how to port it? |
Good idea, I'll do that now. |
|
Thanks! |
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Based on this comment this PR is an alternative to #4. Instead of removing the TinyXML APIs completely, we deprecate them and add the TinyXML2 APIs alongside. That way we won't break everything that depends on us, while still moving forward. Sometime in the future we can actually remove the TinyXML APIs.
Note that I've targeted this at
kinetic-develinstead ofmelodic-devel, since we can add this to kinetic without breaking anything, and thus not branch off. If we decide to go with this PR, then we can delete themelodic-develbranch completely.In addition to this work, this PR also cleans up the code and adds some tests.
@rojkov, @sloretz FYI