From e96ed878e404337fc167f4290822cd9dd054cbd0 Mon Sep 17 00:00:00 2001 From: ephphatha Date: Sun, 27 Aug 2023 20:43:38 +1000 Subject: [PATCH] Use dedicated error enum for parsing failures --- Source/data/file.cpp | 14 +++++++------- Source/data/file.hpp | 2 +- Source/data/iterators.hpp | 35 ++++++++++++++++++++++++++--------- Source/playerdat.cpp | 8 ++++---- test/data_file_test.cpp | 18 +++++++++++++----- 5 files changed, 51 insertions(+), 26 deletions(-) diff --git a/Source/data/file.cpp b/Source/data/file.cpp index 242f0d920a7..98aeeea8876 100644 --- a/Source/data/file.cpp +++ b/Source/data/file.cpp @@ -47,24 +47,24 @@ void DataFile::reportFatalError(Error code, std::string_view fileName) } } -void DataFile::reportFatalFieldError(std::errc code, std::string_view fileName, std::string_view fieldName, const DataFileField &field) +void DataFile::reportFatalFieldError(DataFileField::Error code, std::string_view fileName, std::string_view fieldName, const DataFileField &field) { switch (code) { - case std::errc::invalid_argument: + case DataFileField::Error::NotANumber: app_fatal(fmt::format(fmt::runtime(_( /* TRANSLATORS: Error message when parsing a data file and a text value is encountered when a number is expected. Arguments are {found value}, {column heading}, {file name}, {row/record number}, {column/field number} */ "Non-numeric value {0} for {1} in {2} at row {3} and column {4}")), field.currentValue(), fieldName, fileName, field.row(), field.column())); - case std::errc::result_out_of_range: + case DataFileField::Error::OutOfRange: app_fatal(fmt::format(fmt::runtime(_( /* TRANSLATORS: Error message when parsing a data file and we find a number larger than expected. Arguments are {found value}, {column heading}, {file name}, {row/record number}, {column/field number} */ "Out of range value {0} for {1} in {2} at row {3} and column {4}")), field.currentValue(), fieldName, fileName, field.row(), field.column())); - default: + case DataFileField::Error::InvalidValue: app_fatal(fmt::format(fmt::runtime(_( - /* TRANSLATORS: Fallback error message when an error occurs while parsing a data file and we can't determine the cause. Arguments are {file name}, {row/record number}, {column/field number} */ - "Unexpected error while reading {0} at row {1} and column {2}")), - fileName, field.row(), field.column())); + /* TRANSLATORS: Error message when we find an unrecognised value in a key column. Arguments are {found value}, {column heading}, {file name}, {row/record number}, {column/field number} */ + "Invalid value {0} for {1} in {2} at row {3} and column {4}")), + field.currentValue(), fieldName, fileName, field.row(), field.column())); } } diff --git a/Source/data/file.hpp b/Source/data/file.hpp index c2b67ee8950..f8eb4e2321e 100644 --- a/Source/data/file.hpp +++ b/Source/data/file.hpp @@ -75,7 +75,7 @@ class DataFile { static tl::expected load(std::string_view path); static void reportFatalError(Error code, std::string_view fileName); - static void reportFatalFieldError(std::errc code, std::string_view fileName, std::string_view fieldName, const DataFileField &field); + static void reportFatalFieldError(DataFileField::Error code, std::string_view fileName, std::string_view fieldName, const DataFileField &field); void resetHeader() { diff --git a/Source/data/iterators.hpp b/Source/data/iterators.hpp index 4bbf35e7907..fc0857c37e3 100644 --- a/Source/data/iterators.hpp +++ b/Source/data/iterators.hpp @@ -16,6 +16,26 @@ class DataFileField { unsigned column_; public: + enum class Error { + NotANumber, + OutOfRange, + InvalidValue + }; + + static tl::expected mapError(std::errc ec) + { + switch (ec) { + case std::errc(): + return {}; + case std::errc::result_out_of_range: + return tl::unexpected { Error::OutOfRange }; + case std::errc::invalid_argument: + return tl::unexpected { Error::NotANumber }; + default: + return tl::unexpected { Error::InvalidValue }; + } + } + DataFileField(GetFieldResult *state, const char *end, unsigned row, unsigned column) : state_(state) , end_(end) @@ -56,10 +76,10 @@ class DataFileField { * use with operator* or repeated calls to parseInt (even with different types). * @tparam T an Integral type supported by std::from_chars * @param destination value to store the result of successful parsing - * @return the error code from std::from_chars + * @return an error code corresponding to the from_chars result if parsing failed */ template - [[nodiscard]] std::errc parseInt(T &destination) + [[nodiscard]] tl::expected parseInt(T &destination) { std::from_chars_result result {}; if (state_->status == GetFieldResult::Status::ReadyToRead) { @@ -74,18 +94,15 @@ class DataFileField { } else { result = std::from_chars(state_->value.data(), end_, destination); } - return result.ec; + + return mapError(result.ec); } template - [[nodiscard]] tl::expected asInt() + [[nodiscard]] tl::expected asInt() { T value = 0; - auto parseIntResult = parseInt(value); - if (parseIntResult == std::errc()) { - return value; - } - return tl::unexpected { parseIntResult }; + return parseInt(value).map([value]() { return value; }); } /** diff --git a/Source/playerdat.cpp b/Source/playerdat.cpp index d9d38568f92..9bbc3aaec1e 100644 --- a/Source/playerdat.cpp +++ b/Source/playerdat.cpp @@ -120,11 +120,11 @@ void ReloadExperienceData() case ExperienceColumn::Level: { auto parseIntResult = field.parseInt(level); - if (parseIntResult != std::errc()) { + if (!parseIntResult.has_value()) { if (*field == "MaxLevel") { skipRecord = true; } else { - DataFile::reportFatalFieldError(parseIntResult, filename, "Level", field); + DataFile::reportFatalFieldError(parseIntResult.error(), filename, "Level", field); } } } break; @@ -132,8 +132,8 @@ void ReloadExperienceData() case ExperienceColumn::Experience: { auto parseIntResult = field.parseInt(experience); - if (parseIntResult != std::errc()) { - DataFile::reportFatalFieldError(parseIntResult, filename, "Experience", field); + if (!parseIntResult.has_value()) { + DataFile::reportFatalFieldError(parseIntResult.error(), filename, "Experience", field); } } break; diff --git a/test/data_file_test.cpp b/test/data_file_test.cpp index 3571486933e..ae261c59f7d 100644 --- a/test/data_file_test.cpp +++ b/test/data_file_test.cpp @@ -214,7 +214,11 @@ TEST(DataFileTest, ParseInt) DataFileField field = *fieldIt; uint8_t shortVal = 5; auto parseIntResult = field.parseInt(shortVal); - EXPECT_EQ(parseIntResult, std::errc::invalid_argument) << "Strings are not uint8_t values"; + if (parseIntResult.has_value()) { + ADD_FAILURE() << "Parsing a string as an int should not succeed"; + } else { + EXPECT_EQ(parseIntResult.error(), DataFileField::Error::NotANumber) << "Strings are not uint8_t values"; + } EXPECT_EQ(shortVal, 5) << "Value is not modified when parsing as uint8_t fails due to non-numeric fields"; EXPECT_EQ(*field, "Sample") << "Should be able to access the field value as a string after failure"; ++fieldIt; @@ -225,7 +229,7 @@ TEST(DataFileTest, ParseInt) field = *fieldIt; shortVal = 5; parseIntResult = field.parseInt(shortVal); - EXPECT_EQ(parseIntResult, std::errc()) << "Expected " << field << "to fit into a uint8_t variable"; + EXPECT_TRUE(parseIntResult.has_value()) << "Expected " << field << " to fit into a uint8_t variable"; EXPECT_EQ(shortVal, 145) << "Parsing should give the expected base 10 value"; EXPECT_EQ(*field, "145") << "Should be able to access the field value as a string even after parsing as an int"; ++fieldIt; @@ -235,11 +239,15 @@ TEST(DataFileTest, ParseInt) // Third field is a number too large for a uint8_t but that fits into an int value field = *fieldIt; parseIntResult = field.parseInt(shortVal); - EXPECT_EQ(parseIntResult, std::errc::result_out_of_range) << "A value too large to fit into a uint8_t variable should report an error"; + if (parseIntResult.has_value()) { + ADD_FAILURE() << "Parsing an int into a short variable should not succeed"; + } else { + EXPECT_EQ(parseIntResult.error(), DataFileField::Error::OutOfRange) << "A value too large to fit into a uint8_t variable should report an error"; + } EXPECT_EQ(shortVal, 145) << "Value is not modified when parsing as uint8_t fails due to out of range value"; int longVal = 42; parseIntResult = field.parseInt(longVal); - EXPECT_EQ(parseIntResult, std::errc()) << "Expected " << field << "to fit into an int variable"; + EXPECT_TRUE(parseIntResult.has_value()) << "Expected " << field << " to fit into an int variable"; EXPECT_EQ(longVal, 70322) << "Value is expected to be parsed into a larger type after an out of range failure"; EXPECT_EQ(*field, "70322") << "Should be able to access the field value as a string after parsing as an int"; ++fieldIt; @@ -249,7 +257,7 @@ TEST(DataFileTest, ParseInt) // Fourth field is not an integer, but a value that starts with one or more digits that fit into an uint8_t value field = *fieldIt; parseIntResult = field.parseInt(shortVal); - EXPECT_EQ(parseIntResult, std::errc()) << "Expected " << field << "to fit into a uint8_t variable (even though it's not really an int)"; + EXPECT_TRUE(parseIntResult.has_value()) << "Expected " << field << " to fit into a uint8_t variable (even though it's not really an int)"; EXPECT_EQ(shortVal, 6) << "Value is loaded as expected until the first non-digit character"; EXPECT_EQ(*field, "6.34") << "Should be able to access the field value as a string after failure"; }