Skip to content

Commit

Permalink
Go back to SDF_ASSERT instead of FATAL_ERROR (#1235)
Browse files Browse the repository at this point in the history 
It seems quite hard to guarantee the same flow of the execution of SDF_ASSERT when using a FATAL_ERROR instead. Because of this, this PR reverts all previous SDF_ASSERT changes into FATAL_ERROR


Signed-off-by: Marco A. Gutierrez <[email protected]>
Co-authored-by: Addisu Z. Taddese <[email protected]>
  • Loading branch information
@azeey
Marco A. Gutierrez and azeey authored Feb 10, 2023
1 parent 811760d commit f1d13b4
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 230 deletions.
169 changes: 36 additions & 133 deletions src/Converter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,8 @@ void UpdatePose(tinyxml2::XMLElement *_elem,
{
std::string poseRelTo = pose->Attribute("relative_to");

if (poseRelTo.compare(0, _modelName.size(), _modelName) != 0)
{
std::stringstream ss;
ss << "Error: Pose attribute 'relative_to' does not start with "
<< _modelName;
_errors.push_back({ErrorCode::FATAL_ERROR, ss.str()});
return;
}
SDF_ASSERT(poseRelTo.compare(0, _modelName.size(), _modelName) == 0,
"Error: Pose attribute 'relative_to' does not start with " + _modelName);

poseRelTo = poseRelTo.substr(_childNameIdx);
pose->SetAttribute("relative_to", poseRelTo.c_str());
Expand All @@ -90,11 +84,8 @@ bool Converter::Convert(sdf::Errors &_errors,
const ParserConfig &_config,
bool _quiet)
{
if (_doc == nullptr)
{
_errors.push_back({ErrorCode::FATAL_ERROR, "SDF XML doc is NULL"});
return false;
}

SDF_ASSERT(_doc != nullptr, "SDF XML doc is NULL");

tinyxml2::XMLElement *elem = _doc->FirstChildElement("sdf");

Expand Down Expand Up @@ -192,16 +183,8 @@ void Converter::Convert(sdf::Errors &_errors,
tinyxml2::XMLDocument *_convertDoc,
const ParserConfig &_config)
{
if (_doc == NULL)
{
_errors.push_back({ErrorCode::FATAL_ERROR, "SDF XML doc is NULL"});
return;
}
if (_convertDoc == NULL)
{
_errors.push_back({ErrorCode::FATAL_ERROR, "Convert XML doc is NULL"});
return;
}
SDF_ASSERT(_doc != NULL, "SDF XML doc is NULL");
SDF_ASSERT(_convertDoc != NULL, "Convert XML doc is NULL");

ConvertImpl(_doc->FirstChildElement(), _convertDoc->FirstChildElement(),
_config, _errors);
Expand Down Expand Up @@ -246,16 +229,8 @@ void Converter::ConvertImpl(tinyxml2::XMLElement *_elem,
const ParserConfig &_config,
sdf::Errors &_errors)
{
if (_elem == NULL)
{
_errors.push_back({ErrorCode::FATAL_ERROR, "SDF element is NULL"});
return;
}
if (_elem == NULL)
{
_errors.push_back({ErrorCode::FATAL_ERROR, "Convert element is NULL"});
return;
}
SDF_ASSERT(_elem != NULL, "SDF element is NULL");
SDF_ASSERT(_convert != NULL, "Convert element is NULL");

CheckDeprecation(_elem, _convert, _config, _errors);

Expand Down Expand Up @@ -289,15 +264,15 @@ void Converter::ConvertImpl(tinyxml2::XMLElement *_elem,
}
else if (name == "copy")
{
Move(_elem, childElem, true, _errors);
Move(_elem, childElem, true);
}
else if (name == "map")
{
Map(_elem, childElem, _errors);
}
else if (name == "move")
{
Move(_elem, childElem, false, _errors);
Move(_elem, childElem, false);
}
else if (name == "add")
{
Expand Down Expand Up @@ -329,11 +304,7 @@ void Converter::ConvertImpl(tinyxml2::XMLElement *_elem,
/////////////////////////////////////////////////
void Converter::Unflatten(tinyxml2::XMLElement *_elem, sdf::Errors &_errors)
{
if (_elem == nullptr)
{
_errors.push_back({ErrorCode::FATAL_ERROR, "SDF element is nullptr"});
return;
}
SDF_ASSERT(_elem != nullptr, "SDF element is nullptr");

tinyxml2::XMLDocument *doc = _elem->GetDocument();

Expand Down Expand Up @@ -449,15 +420,9 @@ bool Converter::FindNewModelElements(tinyxml2::XMLElement *_elem,
if (elem->Attribute("attached_to"))
{
attachedTo = elem->Attribute("attached_to");

if (attachedTo.compare(0, newModelNameSize, newModelName) != 0)
{
std::stringstream ss;
ss << "Error: Frame attribute 'attached_to' does not start with "
<< newModelName;
_errors.push_back({ErrorCode::FATAL_ERROR, ss.str()});
return false;
}
SDF_ASSERT(attachedTo.compare(0, newModelNameSize, newModelName) == 0,
"Error: Frame attribute 'attached_to' does not start with " +
newModelName);

// strip new model prefix from attached_to
attachedTo = attachedTo.substr(_childNameIdx);
Expand Down Expand Up @@ -496,14 +461,8 @@ bool Converter::FindNewModelElements(tinyxml2::XMLElement *_elem,
{
eText = e->GetText();

if (eText.compare(0, newModelNameSize, newModelName) != 0)
{
std::stringstream ss;
ss << "Error: Joint's <parent> value does not start with "
<< newModelName;
_errors.push_back({ErrorCode::FATAL_ERROR, ss.str()});
return false;
}
SDF_ASSERT(eText.compare(0, newModelNameSize, newModelName) == 0,
"Error: Joint's <parent> value does not start with " + newModelName);

e->SetText(eText.substr(_childNameIdx).c_str());
}
Expand All @@ -514,14 +473,8 @@ bool Converter::FindNewModelElements(tinyxml2::XMLElement *_elem,
{
eText = e->GetText();

if (eText.compare(0, newModelNameSize, newModelName) != 0)
{
std::stringstream ss;
ss << "Error: Joint's <child> value does not start with "
<< newModelName;
_errors.push_back({ErrorCode::FATAL_ERROR, ss.str()});
return false;
}
SDF_ASSERT(eText.compare(0, newModelNameSize, newModelName) == 0,
"Error: Joint's <child> value does not start with " + newModelName);

e->SetText(eText.substr(_childNameIdx).c_str());
}
Expand All @@ -539,14 +492,10 @@ bool Converter::FindNewModelElements(tinyxml2::XMLElement *_elem,
std::string expressIn =
axisElem->FirstChildElement("xyz")->Attribute("expressed_in");

if (expressIn.compare(0, newModelNameSize, newModelName) != 0)
{
std::stringstream ss;
ss << "Error: <xyz>'s attribute 'expressed_in' does not start "
<< "with " << newModelName;
_errors.push_back({ErrorCode::FATAL_ERROR, ss.str()});
return false;
}
SDF_ASSERT(
expressIn.compare(0, newModelNameSize, newModelName) == 0,
"Error: <xyz>'s attribute 'expressed_in' does not start with " +
newModelName);

expressIn = expressIn.substr(_childNameIdx);

Expand Down Expand Up @@ -608,14 +557,9 @@ bool Converter::FindNewModelElements(tinyxml2::XMLElement *_elem,
{
eText = e->GetText();

if (eText.compare(0, newModelNameSize, newModelName) != 0)
{
std::stringstream ss;
ss << "Error: Gripper's <palm_link> value does not start with "
<< newModelName;
_errors.push_back({ErrorCode::FATAL_ERROR, ss.str()});
return false;
}
SDF_ASSERT(eText.compare(0, newModelNameSize, newModelName) == 0,
"Error: Gripper's <palm_link> value does not start with "
+ newModelName);

e->SetText(eText.substr(_childNameIdx).c_str());
}
Expand All @@ -635,16 +579,8 @@ void Converter::Rename(tinyxml2::XMLElement *_elem,
tinyxml2::XMLElement *_renameElem,
sdf::Errors &_errors)
{
if (_elem == NULL)
{
_errors.push_back({ErrorCode::FATAL_ERROR, "SDF element is NULL"});
return;
}
if (_renameElem == NULL)
{
_errors.push_back({ErrorCode::FATAL_ERROR, "Rename element is NULL"});
return;
}
SDF_ASSERT(_elem != NULL, "SDF element is NULL");
SDF_ASSERT(_renameElem != NULL, "Rename element is NULL");

auto *fromConvertElem = _renameElem->FirstChildElement("from");
auto *toConvertElem = _renameElem->FirstChildElement("to");
Expand Down Expand Up @@ -698,16 +634,8 @@ void Converter::Add(tinyxml2::XMLElement *_elem,
tinyxml2::XMLElement *_addElem,
sdf::Errors &_errors)
{
if (_elem == NULL)
{
_errors.push_back({ErrorCode::FATAL_ERROR, "SDF element is NULL"});
return;
}
if (_addElem == NULL)
{
_errors.push_back({ErrorCode::FATAL_ERROR, "Add element is NULL"});
return;
}
SDF_ASSERT(_elem != NULL, "SDF element is NULL");
SDF_ASSERT(_addElem != NULL, "Add element is NULL");

const char *attributeName = _addElem->Attribute("attribute");
const char *elementName = _addElem->Attribute("element");
Expand Down Expand Up @@ -752,16 +680,8 @@ void Converter::Remove(sdf::Errors &_errors,
tinyxml2::XMLElement *_removeElem,
bool _removeOnlyEmpty)
{
if (_elem == NULL)
{
_errors.push_back({ErrorCode::FATAL_ERROR, "SDF element is NULL"});
return;
}
if (_removeElem == NULL)
{
_errors.push_back({ErrorCode::FATAL_ERROR, "remove element is NULL"});
return;
}
SDF_ASSERT(_elem != NULL, "SDF element is NULL");
SDF_ASSERT(_removeElem != NULL, "remove element is NULL");

const char *attributeName = _removeElem->Attribute("attribute");
const char *elementName = _removeElem->Attribute("element");
Expand Down Expand Up @@ -803,16 +723,8 @@ void Converter::Map(tinyxml2::XMLElement *_elem,
tinyxml2::XMLElement *_mapElem,
sdf::Errors &_errors)
{
if (_elem == nullptr)
{
_errors.push_back({ErrorCode::FATAL_ERROR, "SDF element is nullptr"});
return;
}
if (_mapElem == nullptr)
{
_errors.push_back({ErrorCode::FATAL_ERROR, "Map element is nullptr"});
return;
}
SDF_ASSERT(_elem != nullptr, "SDF element is nullptr");
SDF_ASSERT(_mapElem != nullptr, "Map element is nullptr");

tinyxml2::XMLElement *fromConvertElem = _mapElem->FirstChildElement("from");
tinyxml2::XMLElement *toConvertElem = _mapElem->FirstChildElement("to");
Expand Down Expand Up @@ -1011,19 +923,10 @@ void Converter::Map(tinyxml2::XMLElement *_elem,
/////////////////////////////////////////////////
void Converter::Move(tinyxml2::XMLElement *_elem,
tinyxml2::XMLElement *_moveElem,
const bool _copy,
sdf::Errors &_errors)
const bool _copy)
{
if (_elem == nullptr)
{
_errors.push_back({ErrorCode::FATAL_ERROR, "SDF element is NULL"});
return;
}
if (_moveElem == nullptr)
{
_errors.push_back({ErrorCode::FATAL_ERROR, "Move element is NULL"});
return;
}
SDF_ASSERT(_elem != NULL, "SDF element is NULL");
SDF_ASSERT(_moveElem != NULL, "Move element is NULL");

tinyxml2::XMLElement *fromConvertElem = _moveElem->FirstChildElement("from");
tinyxml2::XMLElement *toConvertElem = _moveElem->FirstChildElement("to");
Expand Down
4 changes: 1 addition & 3 deletions src/Converter.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,9 @@ namespace sdf
/// \param[in] _moveElem A 'convert' element that describes the move
/// operation.
/// \param[in] _copy True to copy the element
/// \param[out] _errors Vector of errors.
private: static void Move(tinyxml2::XMLElement *_elem,
tinyxml2::XMLElement *_moveElem,
const bool _copy,
sdf::Errors &_errors);
const bool _copy);

/// \brief Add an element or attribute to an element.
/// \param[in] _elem The element to receive the value.
Expand Down
43 changes: 9 additions & 34 deletions src/Converter_TEST.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2852,44 +2852,19 @@ TEST(Converter, NullDoc)
tinyxml2::XMLDocument xmlDoc;
tinyxml2::XMLDocument convertXmlDoc;

std::stringstream buffer;
sdf::testing::RedirectConsoleStream redir(
sdf::Console::Instance()->GetMsgStream(), &buffer);

#ifdef _WIN32
sdf::Console::Instance()->SetQuiet(false);
sdf::testing::ScopeExit revertSetQuiet(
[]
{
sdf::Console::Instance()->SetQuiet(true);
});
#endif

sdf::ParserConfig parserConfig;
parserConfig.SetWarningsPolicy(sdf::EnforcementPolicy::ERR);

sdf::Errors errors;
sdf::Converter::Convert(errors, nullptr, &convertXmlDoc, parserConfig);
ASSERT_EQ(errors.size(), 1u);
EXPECT_EQ(errors[0].Code(), sdf::ErrorCode::FATAL_ERROR);
EXPECT_NE(std::string::npos,
errors[0].Message().find("SDF XML doc is NULL"));
errors.clear();

sdf::Converter::Convert(errors, &xmlDoc, nullptr, parserConfig);
ASSERT_EQ(errors.size(), 1u);
EXPECT_EQ(errors[0].Code(), sdf::ErrorCode::FATAL_ERROR);
EXPECT_NE(std::string::npos,
errors[0].Message().find("Convert XML doc is NULL"));
errors.clear();

sdf::Converter::Convert(errors, nullptr, "1.4", parserConfig);
EXPECT_EQ(errors[0].Code(), sdf::ErrorCode::FATAL_ERROR);
EXPECT_NE(std::string::npos,
errors[0].Message().find("SDF XML doc is NULL"));

// Check nothing has been printed
EXPECT_TRUE(buffer.str().empty()) << buffer.str();
ASSERT_THROW(sdf::Converter::Convert(errors, nullptr, &convertXmlDoc,
parserConfig), sdf::AssertionInternalError);
ASSERT_TRUE(errors.empty());
ASSERT_THROW(sdf::Converter::Convert(errors, &xmlDoc, nullptr, parserConfig),
sdf::AssertionInternalError);
ASSERT_TRUE(errors.empty());
ASSERT_THROW(sdf::Converter::Convert(errors, nullptr, "1.4", parserConfig),
sdf::AssertionInternalError);
ASSERT_TRUE(errors.empty());
}

////////////////////////////////////////////////////
Expand Down
Loading

0 comments on commit f1d13b4

Please sign in to comment.