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.

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

Include file path and line number in parsing errors #512

Merged
merged 23 commits into from
Mar 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
938edc0
Simple line number printout for element
aaronchongth Mar 5, 2021
9143ff9
Passing source into readXml, errors in include tags
aaronchongth Mar 8, 2021
efc97fe
Added helper function for creating trace strings
aaronchongth Mar 8, 2021
1e4d1a6
Fixing bad calls to elements in includes
aaronchongth Mar 8, 2021
fe0829c
Fixed bad logic in file exists check
aaronchongth Mar 8, 2021
38db358
Pimpl-ized Error, added API for file path and line number
aaronchongth Mar 12, 2021
618b576
Started using new Error API, removed previous helper functions
aaronchongth Mar 12, 2021
95470f3
Modified error stream to look like before, using << operator for cmdC…
aaronchongth Mar 15, 2021
09e3c2b
Added tests to check for file tracing and line number
aaronchongth Mar 15, 2021
afa7e5d
Added Error constructor without LineNumber for missing elements
aaronchongth Mar 15, 2021
b9605a1
Added tests for parsing errors includes, test sdfs and models
aaronchongth Mar 15, 2021
f273dbe
code_check fixes
aaronchongth Mar 15, 2021
8213b05
Removed accidental + char
aaronchongth Mar 15, 2021
1349ac9
Removed accidental + char
aaronchongth Mar 15, 2021
648d0dd
Merge branch 'aaron/trace_parsing_failure' of ssh://github.com/osrf/s…
aaronchongth Mar 15, 2021
5f4b61d
Remove file and line number reporting in warnings
aaronchongth Mar 15, 2021
bb56a0b
Removed checking for front Error: tag as ign.cc uses << now
aaronchongth Mar 16, 2021
1864331
Removed #ifdefs
aaronchongth Mar 19, 2021
3afb61a
Using std optional for getters
aaronchongth Mar 19, 2021
8905458
Added line number to Error for missing required attribute
aaronchongth Mar 19, 2021
6a0cfa3
Removed unused declaration
aaronchongth Mar 19, 2021
c0ddc7a
enforceConfigurablePolicyCondition accepts Error now, warn and log gr…
aaronchongth Mar 19, 2021
719b5fc
Merge branch 'master' into aaron/trace_parsing_failure
aaronchongth Mar 19, 2021
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
76 changes: 57 additions & 19 deletions include/sdf/Error.hh
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

#include <iostream>
#include <string>
#include <optional>
#include <ignition/utils/ImplPtr.hh>
#include <sdf/sdf_config.h>
#include "sdf/system_util.hh"

Expand Down Expand Up @@ -145,15 +147,33 @@ namespace sdf

class SDFORMAT_VISIBLE Error
{
/// \brief default constructor
public: Error() = default;
/// \brief Default constructor
public: Error();

/// \brief Constructor.
/// \param[in] _code The error code.
/// \param[in] _message A description of the error.
/// \sa ErrorCode.
public: Error(const ErrorCode _code, const std::string &_message);

/// \brief Constructor.
/// \param[in] _code The error code.
/// \param[in] _message A description of the error.
/// \param[in] _filePath The file path that is related to this error.
/// \sa ErrorCode.
public: Error(const ErrorCode _code, const std::string &_message,
const std::string &_filePath);

/// \brief Constructor.
/// \param[in] _code The error code.
/// \param[in] _message A description of the error.
/// \param[in] _filePath The file path that is related to this error.
/// \param[in] _lineNumber The line number in the provided file path where
/// this error was raised.
/// \sa ErrorCode.
public: Error(const ErrorCode _code, const std::string &_message,
const std::string &_filePath, int _lineNumber);

/// \brief Get the error code.
/// \return An error code.
/// \sa ErrorCode.
Expand All @@ -163,6 +183,15 @@ namespace sdf
/// \return Error message.
public: std::string Message() const;

/// \brief Get the file path associated with this error.
/// \return Returns the path of the file that this error is related to.
/// nullopt otherwise.
public: std::optional<std::string> FilePath() const;

/// \brief Get the line number associated with this error.
/// \return Returns the line number. nullopt otherwise.
public: std::optional<int> LineNumber() const;

/// \brief Safe bool conversion.
/// \return True if this Error's Code() != NONE. In otherwords, this is
/// true when there is an error.
Expand All @@ -184,26 +213,35 @@ namespace sdf
public: friend std::ostream &operator<<(std::ostream &_out,
const sdf::Error &_err)
{
_out << "Error Code "
<< static_cast<std::underlying_type<sdf::ErrorCode>::type>(_err.Code())
<< " Msg: " << _err.Message();
if (!_err.FilePath().has_value())
{
_out << "Error Code "
<< static_cast<std::underlying_type<sdf::ErrorCode>::type>(
_err.Code())
<< " Msg: " << _err.Message();
}
else if (!_err.LineNumber().has_value())
{
_out << "Error Code "
<< static_cast<std::underlying_type<sdf::ErrorCode>::type>(
_err.Code())
<< ": [" << _err.FilePath().value() << "]: "
<< " Msg: " << _err.Message();
}
else
{
_out << "Error Code "
<< static_cast<std::underlying_type<sdf::ErrorCode>::type>(
_err.Code())
<< ": [" << _err.FilePath().value() << ":L"
<< std::to_string(_err.LineNumber().value()) << "]: "
<< " Msg: " << _err.Message();
}
return _out;
}

/// \brief The error code value.
private: ErrorCode code = ErrorCode::NONE;

#ifdef _WIN32
// Disable warning C4251 which is triggered by
// std::string
#pragma warning(push)
#pragma warning(disable: 4251)
#endif
/// \brief Description of the error.
private: std::string message = "";
#ifdef _WIN32
#pragma warning(pop)
#endif
/// \brief Private data pointer.
IGN_UTILS_IMPL_PTR(dataPtr)
};
}
}
Expand Down
68 changes: 61 additions & 7 deletions src/Error.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,35 +19,89 @@

using namespace sdf;

class sdf::Error::Implementation
{
/// \brief The error code value.
public: ErrorCode code = ErrorCode::NONE;

/// \brief Description of the error.
public: std::string message = "";

/// \brief File path where the error was raised.
public: std::optional<std::string> filePath = std::nullopt;

/// \brief Line number in the file path where the error was raised.
public: std::optional<int> lineNumber = std::nullopt;
};

/////////////////////////////////////////////////
Error::Error()
: dataPtr(ignition::utils::MakeImpl<Implementation>())
{}

/////////////////////////////////////////////////
Error::Error(const ErrorCode _code, const std::string &_message)
: dataPtr(ignition::utils::MakeImpl<Implementation>())
{
this->dataPtr->code = _code;
this->dataPtr->message = _message;
}

/////////////////////////////////////////////////
Error::Error(const ErrorCode _code, const std::string &_message,
const std::string &_filePath)
: dataPtr(ignition::utils::MakeImpl<Implementation>())
{
this->code = _code;
this->message = _message;
this->dataPtr->code = _code;
this->dataPtr->message = _message;
this->dataPtr->filePath = _filePath;
}

/////////////////////////////////////////////////
Error::Error(const ErrorCode _code, const std::string &_message,
const std::string &_filePath, int _lineNumber)
: dataPtr(ignition::utils::MakeImpl<Implementation>())
{
this->dataPtr->code = _code;
this->dataPtr->message = _message;
this->dataPtr->filePath = _filePath;
this->dataPtr->lineNumber = _lineNumber;
}

/////////////////////////////////////////////////
ErrorCode Error::Code() const
{
return this->code;
return this->dataPtr->code;
}

/////////////////////////////////////////////////
std::string Error::Message() const
{
return this->message;
return this->dataPtr->message;
}

/////////////////////////////////////////////////
std::optional<std::string> Error::FilePath() const
{
return this->dataPtr->filePath;
}

/////////////////////////////////////////////////
std::optional<int> Error::LineNumber() const
{
return this->dataPtr->lineNumber;
}

/////////////////////////////////////////////////
// cppcheck-suppress unusedFunction
Error::operator bool() const
{
return this->code != ErrorCode::NONE;
return this->dataPtr->code != ErrorCode::NONE;
}

/////////////////////////////////////////////////
bool Error::operator==(const bool _value) const
{
return ((this->code != ErrorCode::NONE) && _value) ||
((this->code == ErrorCode::NONE) && !_value);
return ((this->dataPtr->code != ErrorCode::NONE) && _value) ||
((this->dataPtr->code == ErrorCode::NONE) && !_value);
}
25 changes: 24 additions & 1 deletion src/Error_TEST.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/

#include <gtest/gtest.h>
#include <optional>
#include "sdf/sdf_config.h"
#include "sdf/Error.hh"

Expand All @@ -26,18 +27,40 @@ TEST(Error, DefaultConstruction)
EXPECT_EQ(error, false);
EXPECT_EQ(error.Code(), sdf::ErrorCode::NONE);
EXPECT_TRUE(error.Message().empty());
EXPECT_FALSE(error.FilePath().has_value());
EXPECT_FALSE(error.LineNumber().has_value());

if (error)
FAIL();
}

/////////////////////////////////////////////////
TEST(Error, ValueConstruction)
TEST(Error, ValueConstructionWithoutFile)
{
sdf::Error error(sdf::ErrorCode::FILE_READ, "Unable to read a file");
EXPECT_EQ(error, true);
EXPECT_EQ(error.Code(), sdf::ErrorCode::FILE_READ);
EXPECT_EQ(error.Message(), "Unable to read a file");
EXPECT_FALSE(error.FilePath().has_value());
EXPECT_FALSE(error.LineNumber().has_value());

if (!error)
FAIL();
}

/////////////////////////////////////////////////
TEST(Error, ValueConstructionWithFile)
{
const std::string emptyFilePath = "Empty/file/path";
sdf::Error error(sdf::ErrorCode::FILE_READ, "Unable to read a file",
emptyFilePath, 10);
EXPECT_EQ(error, true);
EXPECT_EQ(error.Code(), sdf::ErrorCode::FILE_READ);
EXPECT_EQ(error.Message(), "Unable to read a file");
EXPECT_TRUE(error.FilePath().has_value());
EXPECT_EQ(error.FilePath().value(), emptyFilePath);
EXPECT_TRUE(error.LineNumber().has_value());
EXPECT_EQ(error.LineNumber().value(), 10);

if (!error)
FAIL();
Expand Down
25 changes: 20 additions & 5 deletions src/Utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -92,20 +92,35 @@ bool isValidFrameReference(const std::string &_name)

void enforceConfigurablePolicyCondition(
const sdf::EnforcementPolicy _policy,
const std::string &_message,
const ErrorCode _error,
const sdf::Error &_error,
sdf::Errors &_errors)
{
switch (_policy)
{
case EnforcementPolicy::ERR:
_errors.push_back({_error, _message});
_errors.push_back(_error);
break;
case EnforcementPolicy::WARN:
sdfwarn << _message;
if (!_error.FilePath().has_value())
sdfwarn << _error.Message();
else if (!_error.LineNumber().has_value())
sdfwarn << "[" << _error.FilePath().value() << "]: "
<< _error.Message();
else
sdfwarn << "[" << _error.FilePath().value() << ":L"
<< _error.LineNumber().value() << "]: "
<< _error.Message();
break;
case EnforcementPolicy::LOG:
sdfdbg << _message;
if (!_error.FilePath().has_value())
sdfdbg << _error.Message();
else if (!_error.LineNumber().has_value())
sdfdbg << "[" << _error.FilePath().value() << "]: "
<< _error.Message();
else
sdfdbg << "[" << _error.FilePath().value() << ":L"
<< _error.LineNumber().value() << "]: "
<< _error.Message();
break;
default:
throw std::runtime_error("Unhandled warning policy enum value");
Expand Down
6 changes: 2 additions & 4 deletions src/Utils.hh
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,11 @@ namespace sdf
/// Based on the policy, this will either add it to an errors vector, stream
/// it to sdfwarn, or sdfdbg.
/// \param[in] _policy The enforcement policy to follow
/// \param[in] _message The message to add for this warning
/// \param[in] _error An error code to use if the policy is ERR
/// \param[in] _error An error object to use if the policy is ERR
/// \param[out] _errors The errors to append to if the policy is ERR
void enforceConfigurablePolicyCondition(
const sdf::EnforcementPolicy _policy,
const std::string &_message,
const ErrorCode _error,
const sdf::Error &_error,
sdf::Errors &_errors);

/// \brief Load all objects of a specific sdf element type. No error
Expand Down
24 changes: 24 additions & 0 deletions src/Utils_TEST.cc
Original file line number Diff line number Diff line change
Expand Up @@ -122,3 +122,27 @@ TEST(DOMUtils, ReservedNames)
EXPECT_TRUE(sdf::isReservedName("__world__"));
EXPECT_TRUE(sdf::isReservedName("__anything__"));
}

/////////////////////////////////////////////////
TEST(PolicyUtils, EnforcementPolicyErrors)
{
sdf::Errors errors;
const std::string emptyFilePath = "Empty/file/path";
sdf::Error error(sdf::ErrorCode::FILE_READ, "Unable to read a file",
emptyFilePath, 10);
ASSERT_EQ(error, true);
ASSERT_TRUE(errors.empty());

sdf::enforceConfigurablePolicyCondition(
sdf::EnforcementPolicy::ERR,
error,
errors);
ASSERT_FALSE(errors.empty());
EXPECT_EQ(errors[0], true);
EXPECT_EQ(errors[0].Code(), sdf::ErrorCode::FILE_READ);
EXPECT_EQ(errors[0].Message(), "Unable to read a file");
EXPECT_TRUE(errors[0].FilePath().has_value());
EXPECT_EQ(errors[0].FilePath().value(), emptyFilePath);
EXPECT_TRUE(errors[0].LineNumber().has_value());
EXPECT_EQ(errors[0].LineNumber().value(), 10);
}
2 changes: 1 addition & 1 deletion src/ign.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ extern "C" SDFORMAT_VISIBLE int cmdCheck(const char *_path)
{
for (auto &error : errors)
{
std::cerr << "Error: " << error.Message() << std::endl;
std::cerr << error << std::endl;
}
return -1;
}
Expand Down
Loading