From 808629395008f1b868df975554cb30250438b7e6 Mon Sep 17 00:00:00 2001
From: Michael Carroll <michael@openrobotics.org>
Date: Thu, 11 Jun 2020 10:35:49 -0500
Subject: [PATCH] Address reviewer feedback

Signed-off-by: Michael Carroll <michael@openrobotics.org>
---
 src/CMakeLists.txt      |  4 ----
 src/XmlUtils.cc         | 34 +---------------------------------
 src/XmlUtils.hh         |  5 -----
 src/parser.cc           |  4 ++--
 src/parser_urdf.cc      |  6 +++---
 src/parser_urdf.hh      |  2 --
 src/parser_urdf_TEST.cc |  1 -
 7 files changed, 6 insertions(+), 50 deletions(-)

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index c1d0d6dd4..d4b24977f 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -8,10 +8,6 @@ if (NOT USE_INTERNAL_URDF)
   link_directories(${URDF_LIBRARY_DIRS})
 endif()
 
-# TODO(mjcarroll) These should probably actually be target_* variants
-link_directories(${tinyxml2_LIBRARY_DIRS})
-include_directories(${tinyxml2_INCLUDE_DIRS})
-
 set (sources
   Actor.cc
   AirPressure.cc
diff --git a/src/XmlUtils.cc b/src/XmlUtils.cc
index 5e715b6f1..52bfe964a 100644
--- a/src/XmlUtils.cc
+++ b/src/XmlUtils.cc
@@ -56,38 +56,6 @@ tinyxml2::XMLNode* DeepClone(tinyxml2::XMLDocument *_doc,
   return copy;
 }
 
-/////////////////////////////////////////////////
-std::string &TrimStringLeft(std::string &_s)
-{
-  _s.erase(
-    _s.begin(),
-    find_if_not(_s.begin(), _s.end(),
-      [](int c)
-      {
-        return isspace(c);
-      }));
-  return _s;
-}
-
-/////////////////////////////////////////////////
-std::string &TrimStringRight(std::string &_s)
-{
-  _s.erase(
-    find_if_not(_s.rbegin(), _s.rend(),
-        [](int c)
-        {
-          return isspace(c);
-        }).base(),
-    _s.end());
-  return _s;
-}
-
-/////////////////////////////////////////////////
-std::string TrimString(const std::string &_s)
-{
-  std::string t = _s;
-  return TrimStringLeft(TrimStringRight(t));
-}
-}
 }
+}  // namespace sdf
 
diff --git a/src/XmlUtils.hh b/src/XmlUtils.hh
index cfea11ed3..97f2a5dfa 100644
--- a/src/XmlUtils.hh
+++ b/src/XmlUtils.hh
@@ -32,11 +32,6 @@ namespace sdf
   /// \returns The newly copied node
   tinyxml2::XMLNode* DeepClone(tinyxml2::XMLDocument *_doc,
                                const tinyxml2::XMLNode *_src);
-
-  /// \brief Trim whitespace from a string
-  /// \param[in] _s String to trim
-  /// \returns String _s with whitespace trimmed from both ends
-  std::string TrimString(const std::string &_s);
   }
 }
 #endif
diff --git a/src/parser.cc b/src/parser.cc
index aeb64908d..17269897b 100644
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -83,7 +83,7 @@ template <typename TPtr>
 static inline bool _initFile(const std::string &_filename, TPtr _sdf)
 {
   tinyxml2::XMLDocument xmlDoc;
-  if (xmlDoc.LoadFile(_filename.c_str()))
+  if (tinyxml2::XML_SUCCESS != xmlDoc.LoadFile(_filename.c_str()))
   {
     sdferr << "Unable to load file["
            << _filename << "]: " << xmlDoc.ErrorStr() << "\n";
@@ -756,7 +756,7 @@ std::string getModelFilePath(const std::string &_modelDirPath)
   }
 
   tinyxml2::XMLDocument configFileDoc;
-  if (!configFileDoc.LoadFile(configFilePath.c_str()))
+  if (tinyxml2::XML_SUCCESS != configFileDoc.LoadFile(configFilePath.c_str()))
   {
     sdferr << "Error parsing XML in file ["
            << configFilePath << "]: "
diff --git a/src/parser_urdf.cc b/src/parser_urdf.cc
index d366e4014..c10da0e8e 100644
--- a/src/parser_urdf.cc
+++ b/src/parser_urdf.cc
@@ -261,7 +261,7 @@ bool URDF2SDF::IsURDF(const std::string &_filename)
 {
   tinyxml2::XMLDocument xmlDoc;
 
-  if (!xmlDoc.LoadFile(_filename.c_str()))
+  if (tinyxml2::XML_SUCCESS == xmlDoc.LoadFile(_filename.c_str()))
   {
     tinyxml2::XMLPrinter printer;
     xmlDoc.Print(&printer);
@@ -1157,7 +1157,7 @@ void AddKeyValue(tinyxml2::XMLElement *_elem, const std::string &_key,
     _elem->DeleteChild(childElem);  // remove old _elem
   }
 
-  auto* doc = _elem->GetDocument();
+  auto *doc = _elem->GetDocument();
   tinyxml2::XMLElement *ekey = doc->NewElement(_key.c_str());
   tinyxml2::XMLText *textEkey = doc->NewText(_value.c_str());
   ekey->LinkEndChild(textEkey);
@@ -1196,7 +1196,7 @@ std::string GetKeyValueAsString(tinyxml2::XMLElement* _elem)
       sdfwarn << "Attribute value string not set\n";
     }
   }
-  return TrimString(valueStr);
+  return trim(valueStr.c_str());
 }
 
 /////////////////////////////////////////////////
diff --git a/src/parser_urdf.hh b/src/parser_urdf.hh
index c09214a5e..8f703169c 100644
--- a/src/parser_urdf.hh
+++ b/src/parser_urdf.hh
@@ -54,7 +54,6 @@ namespace sdf
     /// \brief convert urdf file to sdf xml document
     /// \param[in] _urdfStr a string containing filename of the urdf model.
     /// \param[inout] _sdfXmlDoc document to populate with the sdf model.
-    /// \return a tinyxml document containing sdf of the model
     public: void InitModelFile(const std::string &_filename,
                                tinyxml2::XMLDocument *_sdfXmlDoc);
 
@@ -63,7 +62,6 @@ namespace sdf
     /// \param[in] _urdfStr a string containing model urdf
     /// \param[inout] _sdfXmlDoc document to populate with the sdf model.
     /// \param[in] _enforceLimits option to enforce joint limits
-    /// \return a tinyxml document containing sdf of the model
     public: void InitModelString(const std::string &_urdfStr,
                                  tinyxml2::XMLDocument *_sdfXmlDoc,
                                  bool _enforceLimits = true);
diff --git a/src/parser_urdf_TEST.cc b/src/parser_urdf_TEST.cc
index f072cb2cc..c9d5a6ada 100644
--- a/src/parser_urdf_TEST.cc
+++ b/src/parser_urdf_TEST.cc
@@ -36,7 +36,6 @@ std::string getMinimalUrdfTxt()
 std::string convertUrdfStrToSdfStr(const std::string &_urdf)
 {
   sdf::URDF2SDF parser_;
-  SDF_SUPPRESS_DEPRECATED_END
   tinyxml2::XMLDocument sdf_result;
   parser_.InitModelString(_urdf, &sdf_result);
   tinyxml2::XMLPrinter printer;