Skip to content

Commit d087d47

Browse files
azeeyazeey
authored
Refactor sdf::readXml (#681)
Refactors two sections of the the function into helper functions, so that `sdf::readXml` doesn't go over 500 lines. Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
1 parent 1ae08ad commit d087d47

File tree

1 file changed

+120
-91
lines changed

1 file changed

+120
-91
lines changed

‎src/parser.cc

+120-91
Original file line numberDiff line numberDiff line change
@@ -786,48 +786,14 @@ std::string getModelFilePath(const std::string &_modelDirPath)
786786
}
787787

788788
//////////////////////////////////////////////////
789-
bool readXml(TiXmlElement *_xml, ElementPtr _sdf, Errors &_errors)
789+
/// Helper function that reads all the attributes of an element from TinyXML to
790+
/// sdf::Element.
791+
/// \param[in] _xml Pointer to XML element to read the attributes from.
792+
/// \param[in,out] _sdf sdf::Element pointer to parse the attribute data into.
793+
/// \param[out] _errors Captures errors found during parsing.
794+
/// \return True on success, false on error.
795+
static bool readAttributes(TiXmlElement *_xml, ElementPtr _sdf, Errors &_errors)
790796
{
791-
// Check if the element pointer is deprecated.
792-
if (_sdf->GetRequired() == "-1")
793-
{
794-
_errors.push_back({ErrorCode::ELEMENT_DEPRECATED,
795-
"SDF Element[" + _sdf->GetName() + "] is deprecated"});
796-
return true;
797-
}
798-
799-
if (!_xml)
800-
{
801-
if (_sdf->GetRequired() == "1" || _sdf->GetRequired() =="+")
802-
{
803-
_errors.push_back({ErrorCode::ELEMENT_MISSING,
804-
"SDF Element<" + _sdf->GetName() + "> is missing"});
805-
return false;
806-
}
807-
else
808-
{
809-
return true;
810-
}
811-
}
812-
813-
if (_xml->GetText() != nullptr && _sdf->GetValue())
814-
{
815-
if (!_sdf->GetValue()->SetFromString(_xml->GetText()))
816-
return false;
817-
}
818-
819-
// check for nested sdf
820-
std::string refSDFStr = _sdf->ReferenceSDF();
821-
if (!refSDFStr.empty())
822-
{
823-
ElementPtr refSDF;
824-
refSDF.reset(new Element);
825-
std::string refFilename = refSDFStr + ".sdf";
826-
initFile(refFilename, refSDF);
827-
_sdf->RemoveFromParent();
828-
_sdf->Copy(refSDF);
829-
}
830-
831797
TiXmlAttribute *attribute = _xml->FirstAttribute();
832798

833799
unsigned int i = 0;
@@ -865,8 +831,8 @@ bool readXml(TiXmlElement *_xml, ElementPtr _sdf, Errors &_errors)
865831
if (i == _sdf->GetAttributeCount())
866832
{
867833
sdfwarn << "XML Attribute[" << attribute->Name()
868-
<< "] in element[" << _xml->Value()
869-
<< "] not defined in SDF, ignoring.\n";
834+
<< "] in element[" << _xml->Value()
835+
<< "] not defined in SDF, ignoring.\n";
870836
}
871837

872838
attribute = attribute->Next();
@@ -885,6 +851,115 @@ bool readXml(TiXmlElement *_xml, ElementPtr _sdf, Errors &_errors)
885851
}
886852
}
887853

854+
return true;
855+
}
856+
857+
//////////////////////////////////////////////////
858+
/// Helper function to resolve file name from an //include/uri element.
859+
/// \param[in] _uriElem Pointer to the //include/uri XML element.
860+
/// \param[out] _fileName Resolved file name.
861+
/// \param[out] _errors Captures errors found during parsing.
862+
/// \return True if the file name is successfully resolved, false on error.
863+
static bool resolveFileNameFromUri(TiXmlElement *_uriElem,
864+
std::string &_fileName, Errors &_errors)
865+
{
866+
if (_uriElem)
867+
{
868+
const std::string uri = _uriElem->GetText();
869+
const std::string modelPath = sdf::findFile(uri, true, true);
870+
871+
// Test the model path
872+
if (modelPath.empty())
873+
{
874+
_errors.push_back(
875+
{ErrorCode::URI_LOOKUP, "Unable to find uri[" + uri + "]"});
876+
877+
size_t modelFound = uri.find("model://");
878+
if (modelFound != 0u)
879+
{
880+
_errors.push_back(
881+
{ErrorCode::URI_INVALID,
882+
"Invalid uri[" + uri + "]. Should be model://" + uri});
883+
}
884+
return false;
885+
}
886+
else
887+
{
888+
if (!sdf::filesystem::is_directory(modelPath))
889+
{
890+
_errors.push_back({ErrorCode::DIRECTORY_NONEXISTANT,
891+
"Directory doesn't exist[" + modelPath + "]"});
892+
return false;
893+
}
894+
}
895+
896+
// Get the config.xml filename
897+
_fileName = getModelFilePath(modelPath);
898+
899+
if (_fileName .empty())
900+
{
901+
_errors.push_back(
902+
{ErrorCode::URI_LOOKUP,
903+
"Unable to resolve uri[" + uri + "] to model path [" + modelPath +
904+
"] since it does not contain a model.config " + "file."});
905+
return false;
906+
}
907+
}
908+
else
909+
{
910+
_errors.push_back({ErrorCode::ATTRIBUTE_MISSING,
911+
"<include> element missing 'uri' attribute"});
912+
return false;
913+
}
914+
return true;
915+
}
916+
917+
//////////////////////////////////////////////////
918+
bool readXml(TiXmlElement *_xml, ElementPtr _sdf, Errors &_errors)
919+
{
920+
// Check if the element pointer is deprecated.
921+
if (_sdf->GetRequired() == "-1")
922+
{
923+
_errors.push_back({ErrorCode::ELEMENT_DEPRECATED,
924+
"SDF Element[" + _sdf->GetName() + "] is deprecated"});
925+
return true;
926+
}
927+
928+
if (!_xml)
929+
{
930+
if (_sdf->GetRequired() == "1" || _sdf->GetRequired() =="+")
931+
{
932+
_errors.push_back({ErrorCode::ELEMENT_MISSING,
933+
"SDF Element<" + _sdf->GetName() + "> is missing"});
934+
return false;
935+
}
936+
else
937+
{
938+
return true;
939+
}
940+
}
941+
942+
if (_xml->GetText() != nullptr && _sdf->GetValue())
943+
{
944+
if (!_sdf->GetValue()->SetFromString(_xml->GetText()))
945+
return false;
946+
}
947+
948+
// check for nested sdf
949+
std::string refSDFStr = _sdf->ReferenceSDF();
950+
if (!refSDFStr.empty())
951+
{
952+
ElementPtr refSDF;
953+
refSDF.reset(new Element);
954+
std::string refFilename = refSDFStr + ".sdf";
955+
initFile(refFilename, refSDF);
956+
_sdf->RemoveFromParent();
957+
_sdf->Copy(refSDF);
958+
}
959+
960+
if (!readAttributes(_xml, _sdf, _errors))
961+
return false;
962+
888963
if (_sdf->GetCopyChildren())
889964
{
890965
copyChildren(_sdf, _xml, false);
@@ -900,55 +975,9 @@ bool readXml(TiXmlElement *_xml, ElementPtr _sdf, Errors &_errors)
900975
{
901976
if (std::string("include") == elemXml->Value())
902977
{
903-
std::string modelPath;
904-
905-
if (elemXml->FirstChildElement("uri"))
906-
{
907-
std::string uri = elemXml->FirstChildElement("uri")->GetText();
908-
modelPath = sdf::findFile(uri, true, true);
909-
910-
// Test the model path
911-
if (modelPath.empty())
912-
{
913-
_errors.push_back({ErrorCode::URI_LOOKUP,
914-
"Unable to find uri[" + uri + "]"});
915-
916-
size_t modelFound = uri.find("model://");
917-
if (modelFound != 0u)
918-
{
919-
_errors.push_back({ErrorCode::URI_INVALID,
920-
"Invalid uri[" + uri + "]. Should be model://" + uri});
921-
}
922-
continue;
923-
}
924-
else
925-
{
926-
if (!sdf::filesystem::is_directory(modelPath))
927-
{
928-
_errors.push_back({ErrorCode::DIRECTORY_NONEXISTANT,
929-
"Directory doesn't exist[" + modelPath + "]"});
930-
continue;
931-
}
932-
}
933-
934-
// Get the config.xml filename
935-
filename = getModelFilePath(modelPath);
936-
937-
if (filename.empty())
938-
{
939-
_errors.push_back({ErrorCode::URI_LOOKUP,
940-
"Unable to resolve uri[" + uri + "] to model path [" +
941-
modelPath + "] since it does not contain a model.config " +
942-
"file."});
943-
continue;
944-
}
945-
}
946-
else
947-
{
948-
_errors.push_back({ErrorCode::ATTRIBUTE_MISSING,
949-
"<include> element missing 'uri' attribute"});
978+
if (!resolveFileNameFromUri(elemXml->FirstChildElement("uri"), filename,
979+
_errors))
950980
continue;
951-
}
952981

953982
// NOTE: sdf::init is an expensive call. For performance reason,
954983
// a new sdf pointer is created here by cloning a fresh sdf template

0 commit comments

Comments
 (0)