moving output params first, removing POINTER_ERROR

Signed-off-by: Marco A. Gutierrez <[email protected]>

include/sdf/Error.hh

@@ -179,9 +179,6 @@ namespace sdf
 
     /// \brief Generic error during parsing.
     PARSING_ERROR,
-
-    /// \brief Error trying to access a pointer.
-    POINTER_ERROR,
   };
 
   class SDFORMAT_VISIBLE Error

include/sdf/parser.hh

@@ -364,14 +364,14 @@ namespace sdf
   std::string getModelFilePath(const std::string &_modelDirPath);
 
   /// \brief Get the file path to the model file
-  /// \param[in] _modelDirPath directory system path of the model
   /// \param[out] _errors Vector of errors.
+  /// \param[in] _modelDirPath directory system path of the model
   /// \return string with the full filesystem path to the best version (greater
   ///         SDF protocol supported by this sdformat version) of the .sdf
   ///         model files hosted by _modelDirPath.
   SDFORMAT_VISIBLE
-  std::string getModelFilePath(const std::string &_modelDirPath,
-                               sdf::Errors &_errors);
+  std::string getModelFilePath(sdf::Errors &_errors,
+                               const std::string &_modelDirPath);
 
   /// \brief Convert an SDF file to a specific SDF version.
   /// \param[in] _filename Name of the SDF file to convert.
@@ -448,11 +448,11 @@ namespace sdf
   /// not empty.
   /// This checks recursively and should check the files exhaustively
   /// rather than terminating early when the first error is found.
-  /// \param[in] _root SDF Root object to check recursively.
   /// \param[out] _errors Vector of errors.
+  /// \param[in] _root SDF Root object to check recursively.
   /// \return True if all models have valid canonical_link attributes.
   SDFORMAT_VISIBLE
-  bool checkCanonicalLinkNames(const sdf::Root *_root, sdf::Errors &_errors);
+  bool checkCanonicalLinkNames(sdf::Errors &_errors, const sdf::Root *_root);
 
   /// \brief For the world and each model, check that the attached_to graphs
   /// build without errors and have no cycles.
@@ -471,11 +471,11 @@ namespace sdf
   /// leads to a model, link, or world frame.
   /// This checks recursively and should check the files exhaustively
   /// rather than terminating early when the first error is found.
-  /// \param[in] _root SDF Root object to check recursively.
   /// \param[out] _errors Vector of errors.
+  /// \param[in] _root SDF Root object to check recursively.
   /// \return True if all attached_to graphs are valid.
   SDFORMAT_VISIBLE
-  bool checkFrameAttachedToGraph(const sdf::Root *_root, sdf::Errors &_errors);
+  bool checkFrameAttachedToGraph(sdf::Errors &_errors, const sdf::Root *_root);
 
   /// \brief Check that for each frame, the attached_to attribute value
   /// does not match its own frame name but does match the name of a
@@ -494,11 +494,11 @@ namespace sdf
   /// not empty.
   /// This checks recursively and should check the files exhaustively
   /// rather than terminating early when the first error is found.
-  /// \param[in] _root SDF Root object to check recursively.
   /// \param[out] _errors Vector of errors.
+  /// \param[in] _root SDF Root object to check recursively.
   /// \return True if all frames have valid attached_to attributes.
   SDFORMAT_VISIBLE
-  bool checkFrameAttachedToNames(const sdf::Root *_root, sdf::Errors &_errors);
+  bool checkFrameAttachedToNames(sdf::Errors &_errors, const sdf::Root *_root);
 
   /// \brief Check that all joints in contained models specify parent
   /// and child link names that match the names of sibling links.
@@ -557,11 +557,11 @@ namespace sdf
   /// leads to a model, link, or world frame.
   /// This checks recursively and should check the files exhaustively
   /// rather than terminating early when the first error is found.
-  /// \param[in] _root SDF Root object to check recursively.
   /// \param[out] _errors Vector of errors.
+  /// \param[in] _root SDF Root object to check recursively.
   /// \return True if all attached_to graphs are valid.
   SDFORMAT_VISIBLE
-  bool checkPoseRelativeToGraph(const sdf::Root *_root, sdf::Errors &_errors);
+  bool checkPoseRelativeToGraph( sdf::Errors &_errors, const sdf::Root *_root);
 
   /// \brief Check that all sibling elements of the same type have unique names.
   /// This checks recursively and should check the files exhaustively
@@ -575,13 +575,13 @@ namespace sdf
   /// \brief Check that all sibling elements of the same type have unique names.
   /// This checks recursively and should check the files exhaustively
   /// rather than terminating early when the first duplicate name is found.
-  /// \param[in] _elem SDF Element to check recursively.
   /// \param[out] _errors Vector of errors.
+  /// \param[in] _elem SDF Element to check recursively.
   /// \return True if all contained elements have do not share a name with
   /// sibling elements of the same type.
   SDFORMAT_VISIBLE
-  bool recursiveSameTypeUniqueNames(sdf::ElementPtr _elem,
-                                    sdf::Errors &_errors);
+  bool recursiveSameTypeUniqueNames(sdf::Errors &_errors,
+                                    sdf::ElementPtr _elem);
 
   /// \brief Check that all sibling elements of the any type have unique names.
   /// This checks recursively and should check the files exhaustively
@@ -595,12 +595,12 @@ namespace sdf
   /// \brief Check that all sibling elements of the any type have unique names.
   /// This checks recursively and should check the files exhaustively
   /// rather than terminating early when the first duplicate name is found.
-  /// \param[in] _elem SDF Element to check recursively.
   /// \param[out] _errors Vector of errors.
+  /// \param[in] _elem SDF Element to check recursively.
   /// \return True if all contained elements have do not share a name with
   /// sibling elements of any type.
   SDFORMAT_VISIBLE
-  bool recursiveSiblingUniqueNames(sdf::ElementPtr _elem, sdf::Errors &_errors);
+  bool recursiveSiblingUniqueNames(sdf::Errors &_errors, sdf::ElementPtr _elem);
 
   /// \brief Check that all sibling elements do not contain the delimiter
   /// double colons '::' in element names, which is reserved for forming scopes
@@ -617,12 +617,12 @@ namespace sdf
   /// in SDFormat 1.8. This checks recursively and should check the files
   /// exhaustively rather than terminating early when the first name
   /// containing '::' is found.
-  /// \param[in] _elem SDF Element to check recursively.
   /// \param[out] _errors Vector of errors.
+  /// \param[in] _elem SDF Element to check recursively.
   /// \return True if all contained element names do not have the delimiter '::'
   SDFORMAT_VISIBLE
-  bool recursiveSiblingNoDoubleColonInNames(sdf::ElementPtr _elem,
-                                            sdf::Errors &_errors);
+  bool recursiveSiblingNoDoubleColonInNames(sdf::Errors &_errors,
+                                            sdf::ElementPtr _elem);
 
   /// \brief Check whether the element should be validated. If this returns
   /// false, validators such as the unique name and reserve name checkers should

src/parser.cc

@@ -817,7 +817,7 @@ bool readFileInternal(const std::string &_filename, const bool _convert,
 
   if (filesystem::is_directory(filename))
   {
-    filename = getModelFilePath(filename, _errors);
+    filename = getModelFilePath(_errors, filename);
   }
 
   if (!filesystem::exists(filename))
@@ -1095,7 +1095,7 @@ bool readDoc(tinyxml2::XMLDocument *_xmlDoc, SDFPtr _sdf,
     // delimiter '::' in element names not allowed in SDFormat >= 1.8
     gz::math::SemanticVersion sdfVersion(_sdf->Root()->OriginalVersion());
     if (sdfVersion >= gz::math::SemanticVersion(1, 8)
-        && !recursiveSiblingNoDoubleColonInNames(_sdf->Root(), _errors))
+        && !recursiveSiblingNoDoubleColonInNames(_errors, _sdf->Root()))
     {
       _errors.push_back({ErrorCode::RESERVED_NAME,
           "Delimiter '::' found in attribute names of element <"
@@ -1189,7 +1189,7 @@ bool readDoc(tinyxml2::XMLDocument *_xmlDoc, ElementPtr _sdf,
     // delimiter '::' in element names not allowed in SDFormat >= 1.8
     gz::math::SemanticVersion sdfVersion(_sdf->OriginalVersion());
     if (sdfVersion >= gz::math::SemanticVersion(1, 8)
-        && !recursiveSiblingNoDoubleColonInNames(_sdf, _errors))
+        && !recursiveSiblingNoDoubleColonInNames(_errors, _sdf))
     {
       _errors.push_back({ErrorCode::RESERVED_NAME,
           "Delimiter '::' found in attribute names of element <" +
@@ -1325,14 +1325,14 @@ std::string getBestSupportedModelVersion(tinyxml2::XMLElement *_modelXML,
 std::string getModelFilePath(const std::string &_modelDirPath)
 {
   sdf::Errors errors;
-  std::string result = getModelFilePath(_modelDirPath, errors);
+  std::string result = getModelFilePath(errors, _modelDirPath);
   sdf::throwOrPrintErrors(errors);
   return result;
 }
 
 //////////////////////////////////////////////////
-std::string getModelFilePath(const std::string &_modelDirPath,
-                             sdf::Errors &_errors)
+std::string getModelFilePath(sdf::Errors &_errors,
+                             const std::string &_modelDirPath)
 {
   std::string configFilePath;
 
@@ -1546,7 +1546,7 @@ static bool resolveFileNameFromUri(tinyxml2::XMLElement *_includeXml,
       if (sdf::filesystem::is_directory(modelPath))
       {
         // Get the model.config filename
-        _fileName = getModelFilePath(modelPath);
+        _fileName = getModelFilePath(_errors, modelPath);
 
         if (_fileName.empty())
         {
@@ -2222,17 +2222,17 @@ sdf::Errors convertString(SDFPtr _sdf, const std::string &_sdfString,
 bool checkCanonicalLinkNames(const sdf::Root *_root)
 {
   sdf::Errors errors;
-  bool result = checkCanonicalLinkNames(_root, errors);
+  bool result = checkCanonicalLinkNames(errors, _root);
   sdf::throwOrPrintErrors(errors);
   return result;
 }
 
 //////////////////////////////////////////////////
-bool checkCanonicalLinkNames(const sdf::Root *_root, sdf::Errors &_errors)
+bool checkCanonicalLinkNames(sdf::Errors &_errors, const sdf::Root *_root)
 {
   if (!_root)
   {
-    _errors.push_back({ErrorCode::POINTER_ERROR, "Error: invalid sdf::Root "
+    _errors.push_back({ErrorCode::FATAL_ERROR, "Error: invalid sdf::Root "
                       "pointer, unable to check canonical link names."});
     return false;
   }
@@ -2277,13 +2277,13 @@ bool checkCanonicalLinkNames(const sdf::Root *_root, sdf::Errors &_errors)
 bool checkFrameAttachedToNames(const sdf::Root *_root)
 {
   sdf::Errors errors;
-  bool result = checkFrameAttachedToNames(_root, errors);
+  bool result = checkFrameAttachedToNames(errors, _root);
   sdf::throwOrPrintErrors(errors);
   return result;
 }
 
 //////////////////////////////////////////////////
-bool checkFrameAttachedToNames(const sdf::Root *_root, sdf::Errors &_errors)
+bool checkFrameAttachedToNames(sdf::Errors &_errors, const sdf::Root *_root)
 {
   bool result = true;
 
@@ -2420,13 +2420,13 @@ bool checkFrameAttachedToNames(const sdf::Root *_root, sdf::Errors &_errors)
 bool recursiveSameTypeUniqueNames(sdf::ElementPtr _elem)
 {
   sdf::Errors errors;
-  bool result = recursiveSameTypeUniqueNames(_elem, errors);
+  bool result = recursiveSameTypeUniqueNames(errors, _elem);
   sdf::throwOrPrintErrors(errors);
   return result;
 }
 
 //////////////////////////////////////////////////
-bool recursiveSameTypeUniqueNames(sdf::ElementPtr _elem, sdf::Errors &_errors)
+bool recursiveSameTypeUniqueNames(sdf::Errors &_errors, sdf::ElementPtr _elem)
 {
   if (!shouldValidateElement(_elem))
     return true;
@@ -2447,7 +2447,7 @@ bool recursiveSameTypeUniqueNames(sdf::ElementPtr _elem, sdf::Errors &_errors)
   sdf::ElementPtr child = _elem->GetFirstElement();
   while (child)
   {
-    result = recursiveSameTypeUniqueNames(child, _errors) && result;
+    result = recursiveSameTypeUniqueNames(_errors, child) && result;
     child = child->GetNextElement();
   }
 
@@ -2458,13 +2458,13 @@ bool recursiveSameTypeUniqueNames(sdf::ElementPtr _elem, sdf::Errors &_errors)
 bool recursiveSiblingUniqueNames(sdf::ElementPtr _elem)
 {
   sdf::Errors errors;
-  bool result = recursiveSiblingUniqueNames(_elem, errors);
+  bool result = recursiveSiblingUniqueNames(errors, _elem);
   sdf::throwOrPrintErrors(errors);
   return result;
 }
 
 //////////////////////////////////////////////////
-bool recursiveSiblingUniqueNames(sdf::ElementPtr _elem, sdf::Errors &_errors)
+bool recursiveSiblingUniqueNames(sdf::Errors &_errors, sdf::ElementPtr _elem)
 {
   if (!shouldValidateElement(_elem))
     return true;
@@ -2482,7 +2482,7 @@ bool recursiveSiblingUniqueNames(sdf::ElementPtr _elem, sdf::Errors &_errors)
   sdf::ElementPtr child = _elem->GetFirstElement();
   while (child)
   {
-    result = recursiveSiblingUniqueNames(child) && result;
+    result = recursiveSiblingUniqueNames(_errors, child) && result;
     child = child->GetNextElement();
   }
 
@@ -2493,14 +2493,14 @@ bool recursiveSiblingUniqueNames(sdf::ElementPtr _elem, sdf::Errors &_errors)
 bool recursiveSiblingNoDoubleColonInNames(sdf::ElementPtr _elem)
 {
   sdf::Errors errors;
-  bool result = recursiveSiblingNoDoubleColonInNames(_elem, errors);
+  bool result = recursiveSiblingNoDoubleColonInNames(errors, _elem);
   sdf::throwOrPrintErrors(errors);
   return result;
 }
 
 //////////////////////////////////////////////////
-bool recursiveSiblingNoDoubleColonInNames(sdf::ElementPtr _elem,
-                                          sdf::Errors &_errors)
+bool recursiveSiblingNoDoubleColonInNames(sdf::Errors &_errors,
+                                          sdf::ElementPtr _elem)
 {
   if (!shouldValidateElement(_elem))
     return true;
@@ -2518,7 +2518,7 @@ bool recursiveSiblingNoDoubleColonInNames(sdf::ElementPtr _elem,
   sdf::ElementPtr child = _elem->GetFirstElement();
   while (child)
   {
-    result = recursiveSiblingNoDoubleColonInNames(child, _errors) && result;
+    result = recursiveSiblingNoDoubleColonInNames(_errors, child) && result;
     child = child->GetNextElement();
   }
 
@@ -2529,13 +2529,13 @@ bool recursiveSiblingNoDoubleColonInNames(sdf::ElementPtr _elem,
 bool checkFrameAttachedToGraph(const sdf::Root *_root)
 {
   sdf::Errors errors;
-  bool result = checkFrameAttachedToGraph(_root, errors);
+  bool result = checkFrameAttachedToGraph(errors, _root);
   sdf::throwOrPrintErrors(errors);
   return result;
 }
 
 //////////////////////////////////////////////////
-bool checkFrameAttachedToGraph(const sdf::Root *_root, sdf::Errors &_errors)
+bool checkFrameAttachedToGraph(sdf::Errors &_errors, const sdf::Root *_root)
 {
   bool result = true;
 
@@ -2622,13 +2622,13 @@ bool checkFrameAttachedToGraph(const sdf::Root *_root, sdf::Errors &_errors)
 bool checkPoseRelativeToGraph(const sdf::Root *_root)
 {
   sdf::Errors errors;
-  bool result = checkPoseRelativeToGraph(_root, errors);
+  bool result = checkPoseRelativeToGraph(errors, _root);
   sdf::throwOrPrintErrors(errors);
   return result;
 }
 
 //////////////////////////////////////////////////
-bool checkPoseRelativeToGraph(const sdf::Root *_root, sdf::Errors &_errors)
+bool checkPoseRelativeToGraph(sdf::Errors &_errors, const sdf::Root *_root)
 {
   bool result = true;
 

test/integration/includes.cc

@@ -345,14 +345,16 @@ TEST(IncludesTest, IncludeModelMissingConfig)
   sdf::Errors errors;
   ASSERT_TRUE(sdf::readString(stream.str(), sdfParsed, errors));
 
-  ASSERT_GE(1u, errors.size());
-  EXPECT_EQ(1u, errors.size());
-  EXPECT_EQ(errors[0].Code(), sdf::ErrorCode::URI_LOOKUP);
-  EXPECT_NE(std::string::npos, errors[0].Message().find(
-      "Unable to resolve uri[box_missing_config] to model path")) << errors[0];
+  EXPECT_EQ(2u, errors.size());
+  EXPECT_EQ(errors[0].Code(), sdf::ErrorCode::FILE_READ);
   EXPECT_NE(std::string::npos, errors[0].Message().find(
+      "Could not find model.config or manifest.xml in")) << errors[0];
+  EXPECT_EQ(errors[1].Code(), sdf::ErrorCode::URI_LOOKUP);
+  EXPECT_NE(std::string::npos, errors[1].Message().find(
+      "Unable to resolve uri[box_missing_config] to model path")) << errors[1];
+  EXPECT_NE(std::string::npos, errors[1].Message().find(
       "box_missing_config] since it does not contain a model.config file"))
-    << errors[0];
+    << errors[1];
 
   sdf::Root root;
   errors = root.Load(sdfParsed);