Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Go back to SDF_ASSERT instead of FATAL_ERROR #1235

Merged
merged 5 commits into from
Feb 10, 2023
Merged

Go back to SDF_ASSERT instead of FATAL_ERROR #1235

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ac2258b
going back to SDF_ASSERT instead of FATAL_ERROR
Feb 8, 2023
917386a
fix line > 80 chars.
Feb 8, 2023
eedac58
adding back RemoveChild with sdf:Errors
Feb 9, 2023
d9474b3
test to make sure both RemoveChild behave the same
Feb 9, 2023
6b9c929
Remove warning for unused sdf::Errors
Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions include/sdf/Element.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,11 +559,6 @@ namespace sdf
/// \param[in] _child Pointer to the child to remove.
public: void RemoveChild(ElementPtr _child);

/// \brief Remove a child element.
/// \param[in] _child Pointer to the child to remove.
/// \param[out] _errors Vector of errors.
public: void RemoveChild(ElementPtr _child, sdf::Errors &_errors);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't remove this because it breaks ABI :(.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh true! shall we deprecate it instead and remove it on the the next release?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't deprecate it either since our policy is to start a deprecation cycle on a new major release. So, if we want to do that, we'd have to do it in main.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll open a PR to main then. Would an sdf::Error with an ErrorCode::WARNING message suffice?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what the ErrorCode::WARNING would be for.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deprecation message?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, we'd use the SDF_DEPRECATED macro for that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GZ_DEPRECATED maybe?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, GZ_DEPRECATED. I guess SDF_DEPRECATED is deprecated 😁

We can discuss this on the followup PR, but if there's a chance the sdf::Error might be used by RemoveChild, it might be better to deprecate the overload that doesn't take sdf::Error.


/// \brief Remove all child elements.
public: void ClearElements();

Expand Down
169 changes: 36 additions & 133 deletions src/Converter.cc
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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