From 6541ac43f5dafa45f9a4f72aa852ea6e33bdffe1 Mon Sep 17 00:00:00 2001 From: elsid Date: Mon, 22 May 2023 01:19:40 +0200 Subject: [PATCH 1/8] Add UniversalId unit tests --- apps/opencs/model/world/universalid.cpp | 4 +- apps/opencs_tests/CMakeLists.txt | 1 + .../model/world/testuniversalid.cpp | 157 ++++++++++++++++++ 3 files changed, 160 insertions(+), 2 deletions(-) create mode 100644 apps/opencs_tests/model/world/testuniversalid.cpp diff --git a/apps/opencs/model/world/universalid.cpp b/apps/opencs/model/world/universalid.cpp index a4afa56706..7fc0b97911 100644 --- a/apps/opencs/model/world/universalid.cpp +++ b/apps/opencs/model/world/universalid.cpp @@ -272,7 +272,7 @@ CSMWorld::UniversalId::UniversalId(Type type, const std::string& id) mClass = sIdArg[i].mClass; return; } - throw std::logic_error("invalid ID argument UniversalId type"); + throw std::logic_error("invalid ID argument UniversalId type: " + std::to_string(type)); } CSMWorld::UniversalId::UniversalId(Type type, const ESM::RefId& id) @@ -292,7 +292,7 @@ CSMWorld::UniversalId::UniversalId(Type type, int index) return; } - throw std::logic_error("invalid index argument UniversalId type"); + throw std::logic_error("invalid index argument UniversalId type: " + std::to_string(type)); } CSMWorld::UniversalId::Class CSMWorld::UniversalId::getClass() const diff --git a/apps/opencs_tests/CMakeLists.txt b/apps/opencs_tests/CMakeLists.txt index 92b14b797f..5d76dc2032 100644 --- a/apps/opencs_tests/CMakeLists.txt +++ b/apps/opencs_tests/CMakeLists.txt @@ -1,6 +1,7 @@ file(GLOB OPENCS_TESTS_SRC_FILES main.cpp model/world/testinfocollection.cpp + model/world/testuniversalid.cpp ) source_group(apps\\openmw-cs-tests FILES ${OPENCS_TESTS_SRC_FILES}) diff --git a/apps/opencs_tests/model/world/testuniversalid.cpp b/apps/opencs_tests/model/world/testuniversalid.cpp new file mode 100644 index 0000000000..a891f7528b --- /dev/null +++ b/apps/opencs_tests/model/world/testuniversalid.cpp @@ -0,0 +1,157 @@ +#include "apps/opencs/model/world/universalid.hpp" + +#include +#include + +#include + +namespace CSMWorld +{ + namespace + { + using namespace ::testing; + + TEST(CSMWorldUniversalIdTest, shouldFailToConstructFromNoneWithInvalidType) + { + EXPECT_THROW( + UniversalId{ static_cast(std::numeric_limits::max()) }, std::logic_error); + } + + TEST(CSMWorldUniversalIdTest, shouldFailToConstructFromStringWithInvalidType) + { + EXPECT_THROW(UniversalId(UniversalId::Type_Search, "invalid"), std::logic_error); + } + + TEST(CSMWorldUniversalIdTest, shouldFailToConstructFromIntWithInvalidType) + { + EXPECT_THROW(UniversalId(UniversalId::Type_Activator, 42), std::logic_error); + } + + TEST(CSMWorldUniversalIdTest, shouldFailToConstructFromInvalidUniversalIdString) + { + EXPECT_THROW(UniversalId("invalid"), std::runtime_error); + } + + TEST(CSMWorldUniversalIdTest, getIndexShouldThrowExceptionForDefaultConstructed) + { + const UniversalId id; + EXPECT_THROW(id.getIndex(), std::logic_error); + } + + TEST(CSMWorldUniversalIdTest, getIndexShouldThrowExceptionForConstructedFromString) + { + const UniversalId id(UniversalId::Type_Activator, "a"); + EXPECT_THROW(id.getIndex(), std::logic_error); + } + + TEST(CSMWorldUniversalIdTest, getIndexShouldReturnValueForConstructedFromInt) + { + const UniversalId id(UniversalId::Type_Search, 42); + EXPECT_EQ(id.getIndex(), 42); + } + + TEST(CSMWorldUniversalIdTest, getIdShouldThrowExceptionForConstructedFromInt) + { + const UniversalId id(UniversalId::Type_Search, 42); + EXPECT_THROW(id.getId(), std::logic_error); + } + + TEST(CSMWorldUniversalIdTest, getIdShouldReturnValueForConstructedFromString) + { + const UniversalId id(UniversalId::Type_Activator, "a"); + EXPECT_EQ(id.getId(), "a"); + } + + struct Params + { + UniversalId mId; + UniversalId::Type mType; + UniversalId::Class mClass; + UniversalId::ArgumentType mArgumentType; + std::string mTypeName; + std::string mString; + std::string mIcon; + }; + + struct CSMWorldUniversalIdValidPerTypeTest : TestWithParam + { + }; + + TEST_P(CSMWorldUniversalIdValidPerTypeTest, getTypeShouldReturnExpected) + { + EXPECT_EQ(GetParam().mId.getType(), GetParam().mType); + } + + TEST_P(CSMWorldUniversalIdValidPerTypeTest, getClassShouldReturnExpected) + { + EXPECT_EQ(GetParam().mId.getClass(), GetParam().mClass); + } + + TEST_P(CSMWorldUniversalIdValidPerTypeTest, getArgumentTypeShouldReturnExpected) + { + EXPECT_EQ(GetParam().mId.getArgumentType(), GetParam().mArgumentType); + } + + TEST_P(CSMWorldUniversalIdValidPerTypeTest, shouldBeEqualToCopy) + { + EXPECT_EQ(GetParam().mId, UniversalId(GetParam().mId)); + } + + TEST_P(CSMWorldUniversalIdValidPerTypeTest, shouldNotBeLessThanCopy) + { + EXPECT_FALSE(GetParam().mId < UniversalId(GetParam().mId)); + } + + TEST_P(CSMWorldUniversalIdValidPerTypeTest, getTypeNameShouldReturnExpected) + { + EXPECT_EQ(GetParam().mId.getTypeName(), GetParam().mTypeName); + } + + TEST_P(CSMWorldUniversalIdValidPerTypeTest, toStringShouldReturnExpected) + { + EXPECT_EQ(GetParam().mId.toString(), GetParam().mString); + } + + TEST_P(CSMWorldUniversalIdValidPerTypeTest, getIconShouldReturnExpected) + { + EXPECT_EQ(GetParam().mId.getIcon(), GetParam().mIcon); + } + + const std::array validParams = { + Params{ UniversalId(), UniversalId::Type_None, UniversalId::Class_None, UniversalId::ArgumentType_None, "-", + "-", ":placeholder" }, + + Params{ UniversalId(UniversalId::Type_None), UniversalId::Type_None, UniversalId::Class_None, + UniversalId::ArgumentType_None, "-", "-", ":placeholder" }, + Params{ UniversalId(UniversalId::Type_RegionMap), UniversalId::Type_RegionMap, UniversalId::Class_NonRecord, + UniversalId::ArgumentType_None, "Region Map", "Region Map", ":./region-map.png" }, + Params{ UniversalId(UniversalId::Type_RunLog), UniversalId::Type_RunLog, UniversalId::Class_Transient, + UniversalId::ArgumentType_None, "Run Log", "Run Log", ":./run-log.png" }, + Params{ UniversalId(UniversalId::Type_Lands), UniversalId::Type_Lands, UniversalId::Class_RecordList, + UniversalId::ArgumentType_None, "Lands", "Lands", ":./land-heightmap.png" }, + Params{ UniversalId(UniversalId::Type_Icons), UniversalId::Type_Icons, UniversalId::Class_ResourceList, + UniversalId::ArgumentType_None, "Icons", "Icons", ":./resources-icon" }, + + Params{ UniversalId(UniversalId::Type_Activator, "a"), UniversalId::Type_Activator, + UniversalId::Class_RefRecord, UniversalId::ArgumentType_Id, "Activator", "Activator: a", + ":./activator.png" }, + Params{ UniversalId(UniversalId::Type_Gmst, "b"), UniversalId::Type_Gmst, UniversalId::Class_Record, + UniversalId::ArgumentType_Id, "Game Setting", "Game Setting: b", ":./gmst.png" }, + Params{ UniversalId(UniversalId::Type_Mesh, "c"), UniversalId::Type_Mesh, UniversalId::Class_Resource, + UniversalId::ArgumentType_Id, "Mesh", "Mesh: c", ":./resources-mesh" }, + Params{ UniversalId(UniversalId::Type_Scene, "d"), UniversalId::Type_Scene, UniversalId::Class_Collection, + UniversalId::ArgumentType_Id, "Scene", "Scene: d", ":./scene.png" }, + Params{ UniversalId(UniversalId::Type_Reference, "e"), UniversalId::Type_Reference, + UniversalId::Class_SubRecord, UniversalId::ArgumentType_Id, "Instance", "Instance: e", + ":./instance.png" }, + + Params{ UniversalId(UniversalId::Type_Search, 42), UniversalId::Type_Search, UniversalId::Class_Transient, + UniversalId::ArgumentType_Index, "Global Search", "Global Search: 42", ":./menu-search.png" }, + + Params{ UniversalId("Instance: f"), UniversalId::Type_Reference, UniversalId::Class_SubRecord, + UniversalId::ArgumentType_Id, "Instance", "Instance: f", ":./instance.png" }, + }; + + INSTANTIATE_TEST_SUITE_P(ValidParams, CSMWorldUniversalIdValidPerTypeTest, ValuesIn(validParams)); + } +} From f2a3462e599f640d01c078d945439f705d8088db Mon Sep 17 00:00:00 2001 From: elsid Date: Mon, 22 May 2023 02:07:22 +0200 Subject: [PATCH 2/8] Fix UniversalId constructor from ESM::RefId --- apps/opencs/model/world/universalid.cpp | 2 +- apps/opencs_tests/model/world/testuniversalid.cpp | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/apps/opencs/model/world/universalid.cpp b/apps/opencs/model/world/universalid.cpp index 7fc0b97911..a699b6785e 100644 --- a/apps/opencs/model/world/universalid.cpp +++ b/apps/opencs/model/world/universalid.cpp @@ -276,8 +276,8 @@ CSMWorld::UniversalId::UniversalId(Type type, const std::string& id) } CSMWorld::UniversalId::UniversalId(Type type, const ESM::RefId& id) + : UniversalId(type, id.getRefIdString()) { - UniversalId(type, id.getRefIdString()); } CSMWorld::UniversalId::UniversalId(Type type, int index) diff --git a/apps/opencs_tests/model/world/testuniversalid.cpp b/apps/opencs_tests/model/world/testuniversalid.cpp index a891f7528b..93ddd6ce8f 100644 --- a/apps/opencs_tests/model/world/testuniversalid.cpp +++ b/apps/opencs_tests/model/world/testuniversalid.cpp @@ -150,6 +150,10 @@ namespace CSMWorld Params{ UniversalId("Instance: f"), UniversalId::Type_Reference, UniversalId::Class_SubRecord, UniversalId::ArgumentType_Id, "Instance", "Instance: f", ":./instance.png" }, + + Params{ UniversalId(UniversalId::Type_Reference, ESM::RefId::stringRefId("g")), UniversalId::Type_Reference, + UniversalId::Class_SubRecord, UniversalId::ArgumentType_Id, "Instance", "Instance: g", + ":./instance.png" }, }; INSTANTIATE_TEST_SUITE_P(ValidParams, CSMWorldUniversalIdValidPerTypeTest, ValuesIn(validParams)); From 4cd5efc6ee6788cdf6af4b5265ed5aa88f7ae8e2 Mon Sep 17 00:00:00 2001 From: elsid Date: Mon, 22 May 2023 01:12:44 +0200 Subject: [PATCH 3/8] Implement UniversalId with std::variant --- apps/opencs/model/world/universalid.cpp | 123 ++++++++---------------- apps/opencs/model/world/universalid.hpp | 25 +++-- 2 files changed, 52 insertions(+), 96 deletions(-) diff --git a/apps/opencs/model/world/universalid.cpp b/apps/opencs/model/world/universalid.cpp index a699b6785e..790a897df8 100644 --- a/apps/opencs/model/world/universalid.cpp +++ b/apps/opencs/model/world/universalid.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include namespace @@ -178,10 +179,23 @@ namespace // end marker { CSMWorld::UniversalId::Class_None, CSMWorld::UniversalId::Type_None, 0, 0 }, }; + + struct WriteToStream + { + std::ostream& mStream; + + void operator()(std::monostate) const {} + + template + void operator()(const T& value) const + { + mStream << ": " << value; + } + }; } CSMWorld::UniversalId::UniversalId(const std::string& universalId) - : mIndex(0) + : mValue(std::monostate{}) { std::string::size_type index = universalId.find(':'); @@ -192,24 +206,26 @@ CSMWorld::UniversalId::UniversalId(const std::string& universalId) for (int i = 0; sIdArg[i].mName; ++i) if (type == sIdArg[i].mName) { - mArgumentType = ArgumentType_Id; mType = sIdArg[i].mType; mClass = sIdArg[i].mClass; - mId = universalId.substr(index + 2); + mValue = universalId.substr(index + 2); return; } for (int i = 0; sIndexArg[i].mName; ++i) if (type == sIndexArg[i].mName) { - mArgumentType = ArgumentType_Index; mType = sIndexArg[i].mType; mClass = sIndexArg[i].mClass; std::istringstream stream(universalId.substr(index + 2)); - if (stream >> mIndex) + int index = 0; + if (stream >> index) + { + mValue = index; return; + } break; } @@ -219,7 +235,6 @@ CSMWorld::UniversalId::UniversalId(const std::string& universalId) for (int i = 0; sNoArg[i].mName; ++i) if (universalId == sNoArg[i].mName) { - mArgumentType = ArgumentType_None; mType = sNoArg[i].mType; mClass = sNoArg[i].mClass; return; @@ -230,9 +245,8 @@ CSMWorld::UniversalId::UniversalId(const std::string& universalId) } CSMWorld::UniversalId::UniversalId(Type type) - : mArgumentType(ArgumentType_None) - , mType(type) - , mIndex(0) + : mType(type) + , mValue(std::monostate{}) { for (int i = 0; sNoArg[i].mName; ++i) if (type == sNoArg[i].mType) @@ -244,7 +258,7 @@ CSMWorld::UniversalId::UniversalId(Type type) for (int i = 0; sIdArg[i].mName; ++i) if (type == sIdArg[i].mType) { - mArgumentType = ArgumentType_Id; + mValue = std::string(); mClass = sIdArg[i].mClass; return; } @@ -252,7 +266,7 @@ CSMWorld::UniversalId::UniversalId(Type type) for (int i = 0; sIndexArg[i].mName; ++i) if (type == sIndexArg[i].mType) { - mArgumentType = ArgumentType_Index; + mValue = int{}; mClass = sIndexArg[i].mClass; return; } @@ -261,10 +275,8 @@ CSMWorld::UniversalId::UniversalId(Type type) } CSMWorld::UniversalId::UniversalId(Type type, const std::string& id) - : mArgumentType(ArgumentType_Id) - , mType(type) - , mId(id) - , mIndex(0) + : mType(type) + , mValue(id) { for (int i = 0; sIdArg[i].mName; ++i) if (type == sIdArg[i].mType) @@ -281,9 +293,8 @@ CSMWorld::UniversalId::UniversalId(Type type, const ESM::RefId& id) } CSMWorld::UniversalId::UniversalId(Type type, int index) - : mArgumentType(ArgumentType_Index) - , mType(type) - , mIndex(index) + : mType(type) + , mValue(index) { for (int i = 0; sIndexArg[i].mName; ++i) if (type == sIndexArg[i].mType) @@ -302,7 +313,7 @@ CSMWorld::UniversalId::Class CSMWorld::UniversalId::getClass() const CSMWorld::UniversalId::ArgumentType CSMWorld::UniversalId::getArgumentType() const { - return mArgumentType; + return static_cast(mValue.index()); } CSMWorld::UniversalId::Type CSMWorld::UniversalId::getType() const @@ -312,61 +323,24 @@ CSMWorld::UniversalId::Type CSMWorld::UniversalId::getType() const const std::string& CSMWorld::UniversalId::getId() const { - if (mArgumentType != ArgumentType_Id) - throw std::logic_error("invalid access to ID of non-ID UniversalId"); + if (const std::string* result = std::get_if(&mValue)) + return *result; - return mId; + throw std::logic_error("invalid access to ID of non-ID UniversalId"); } int CSMWorld::UniversalId::getIndex() const { - if (mArgumentType != ArgumentType_Index) - throw std::logic_error("invalid access to index of non-index UniversalId"); + if (const int* result = std::get_if(&mValue)) + return *result; - return mIndex; -} - -bool CSMWorld::UniversalId::isEqual(const UniversalId& universalId) const -{ - if (mClass != universalId.mClass || mArgumentType != universalId.mArgumentType || mType != universalId.mType) - return false; - - switch (mArgumentType) - { - case ArgumentType_Id: - return mId == universalId.mId; - case ArgumentType_Index: - return mIndex == universalId.mIndex; - - default: - return true; - } -} - -bool CSMWorld::UniversalId::isLess(const UniversalId& universalId) const -{ - if (mType < universalId.mType) - return true; - - if (mType > universalId.mType) - return false; - - switch (mArgumentType) - { - case ArgumentType_Id: - return mId < universalId.mId; - case ArgumentType_Index: - return mIndex < universalId.mIndex; - - default: - return false; - } + throw std::logic_error("invalid access to index of non-index UniversalId"); } std::string CSMWorld::UniversalId::getTypeName() const { const TypeData* typeData - = mArgumentType == ArgumentType_None ? sNoArg : (mArgumentType == ArgumentType_Id ? sIdArg : sIndexArg); + = getArgumentType() == ArgumentType_None ? sNoArg : (getArgumentType() == ArgumentType_Id ? sIdArg : sIndexArg); for (int i = 0; typeData[i].mName; ++i) if (typeData[i].mType == mType) @@ -381,17 +355,7 @@ std::string CSMWorld::UniversalId::toString() const stream << getTypeName(); - switch (mArgumentType) - { - case ArgumentType_None: - break; - case ArgumentType_Id: - stream << ": " << mId; - break; - case ArgumentType_Index: - stream << ": " << mIndex; - break; - } + std::visit(WriteToStream{ stream }, mValue); return stream.str(); } @@ -399,7 +363,7 @@ std::string CSMWorld::UniversalId::toString() const std::string CSMWorld::UniversalId::getIcon() const { const TypeData* typeData - = mArgumentType == ArgumentType_None ? sNoArg : (mArgumentType == ArgumentType_Id ? sIdArg : sIndexArg); + = getArgumentType() == ArgumentType_None ? sNoArg : (getArgumentType() == ArgumentType_Id ? sIdArg : sIndexArg); for (int i = 0; typeData[i].mName; ++i) if (typeData[i].mType == mType) @@ -463,15 +427,10 @@ CSMWorld::UniversalId::Type CSMWorld::UniversalId::getParentType(Type type) bool CSMWorld::operator==(const CSMWorld::UniversalId& left, const CSMWorld::UniversalId& right) { - return left.isEqual(right); -} - -bool CSMWorld::operator!=(const CSMWorld::UniversalId& left, const CSMWorld::UniversalId& right) -{ - return !left.isEqual(right); + return std::tie(left.mClass, left.mType, left.mValue) == std::tie(right.mClass, right.mType, right.mValue); } bool CSMWorld::operator<(const UniversalId& left, const UniversalId& right) { - return left.isLess(right); + return std::tie(left.mClass, left.mType, left.mValue) < std::tie(right.mClass, right.mType, right.mValue); } diff --git a/apps/opencs/model/world/universalid.hpp b/apps/opencs/model/world/universalid.hpp index c8c3aed587..38404f68eb 100644 --- a/apps/opencs/model/world/universalid.hpp +++ b/apps/opencs/model/world/universalid.hpp @@ -2,6 +2,7 @@ #define CSM_WOLRD_UNIVERSALID_H #include +#include #include #include @@ -31,7 +32,7 @@ namespace CSMWorld { ArgumentType_None, ArgumentType_Id, - ArgumentType_Index + ArgumentType_Index, }; /// \note A record list type must always be immediately followed by the matching @@ -144,14 +145,6 @@ namespace CSMWorld NumberOfTypes = Type_RunLog + 1 }; - private: - Class mClass; - ArgumentType mArgumentType; - Type mType; - std::string mId; - int mIndex; - - public: UniversalId(const std::string& universalId); UniversalId(Type type = Type_None); @@ -176,10 +169,6 @@ namespace CSMWorld int getIndex() const; ///< Calling this function for a non-index type will throw an exception. - bool isEqual(const UniversalId& universalId) const; - - bool isLess(const UniversalId& universalId) const; - std::string getTypeName() const; std::string toString() const; @@ -195,10 +184,18 @@ namespace CSMWorld /// that contains records of type \a type. /// Otherwise return Type_None. static Type getParentType(Type type); + + private: + Class mClass; + Type mType; + std::variant mValue; + + friend bool operator==(const UniversalId& left, const UniversalId& right); + + friend bool operator<(const UniversalId& left, const UniversalId& right); }; bool operator==(const UniversalId& left, const UniversalId& right); - bool operator!=(const UniversalId& left, const UniversalId& right); bool operator<(const UniversalId& left, const UniversalId& right); } From ceab7557f3413cd733a928a24594c502b8a3f881 Mon Sep 17 00:00:00 2001 From: elsid Date: Mon, 22 May 2023 02:17:52 +0200 Subject: [PATCH 4/8] Add rudimentary support for ESM::RefId in UniversalId Ideally std::string support should be removed but this may affect too much code. --- apps/opencs/model/world/universalid.cpp | 33 +++++++++++++++---- apps/opencs/model/world/universalid.hpp | 5 +-- .../model/world/testuniversalid.cpp | 12 ++++++- 3 files changed, 40 insertions(+), 10 deletions(-) diff --git a/apps/opencs/model/world/universalid.cpp b/apps/opencs/model/world/universalid.cpp index 790a897df8..5a74ca57af 100644 --- a/apps/opencs/model/world/universalid.cpp +++ b/apps/opencs/model/world/universalid.cpp @@ -184,7 +184,7 @@ namespace { std::ostream& mStream; - void operator()(std::monostate) const {} + void operator()(std::monostate /*value*/) const {} template void operator()(const T& value) const @@ -192,6 +192,19 @@ namespace mStream << ": " << value; } }; + + struct GetTypeData + { + const TypeData* operator()(std::monostate /*value*/) const { return sNoArg; } + + const TypeData* operator()(int /*value*/) const { return sIndexArg; } + + template + const TypeData* operator()(const T& /*value*/) const + { + return sIdArg; + } + }; } CSMWorld::UniversalId::UniversalId(const std::string& universalId) @@ -287,9 +300,17 @@ CSMWorld::UniversalId::UniversalId(Type type, const std::string& id) throw std::logic_error("invalid ID argument UniversalId type: " + std::to_string(type)); } -CSMWorld::UniversalId::UniversalId(Type type, const ESM::RefId& id) - : UniversalId(type, id.getRefIdString()) +CSMWorld::UniversalId::UniversalId(Type type, ESM::RefId id) + : mType(type) + , mValue(id) { + for (int i = 0; sIdArg[i].mName; ++i) + if (type == sIdArg[i].mType) + { + mClass = sIdArg[i].mClass; + return; + } + throw std::logic_error("invalid RefId argument UniversalId type: " + std::to_string(type)); } CSMWorld::UniversalId::UniversalId(Type type, int index) @@ -339,8 +360,7 @@ int CSMWorld::UniversalId::getIndex() const std::string CSMWorld::UniversalId::getTypeName() const { - const TypeData* typeData - = getArgumentType() == ArgumentType_None ? sNoArg : (getArgumentType() == ArgumentType_Id ? sIdArg : sIndexArg); + const TypeData* typeData = std::visit(GetTypeData{}, mValue); for (int i = 0; typeData[i].mName; ++i) if (typeData[i].mType == mType) @@ -362,8 +382,7 @@ std::string CSMWorld::UniversalId::toString() const std::string CSMWorld::UniversalId::getIcon() const { - const TypeData* typeData - = getArgumentType() == ArgumentType_None ? sNoArg : (getArgumentType() == ArgumentType_Id ? sIdArg : sIndexArg); + const TypeData* typeData = std::visit(GetTypeData{}, mValue); for (int i = 0; typeData[i].mName; ++i) if (typeData[i].mType == mType) diff --git a/apps/opencs/model/world/universalid.hpp b/apps/opencs/model/world/universalid.hpp index 38404f68eb..8ade978a5c 100644 --- a/apps/opencs/model/world/universalid.hpp +++ b/apps/opencs/model/world/universalid.hpp @@ -33,6 +33,7 @@ namespace CSMWorld ArgumentType_None, ArgumentType_Id, ArgumentType_Index, + ArgumentType_RefId, }; /// \note A record list type must always be immediately followed by the matching @@ -152,7 +153,7 @@ namespace CSMWorld UniversalId(Type type, const std::string& id); ///< Using a type for a non-ID-argument UniversalId will throw an exception. - UniversalId(Type type, const ESM::RefId& id); + UniversalId(Type type, ESM::RefId id); UniversalId(Type type, int index); ///< Using a type for a non-index-argument UniversalId will throw an exception. @@ -188,7 +189,7 @@ namespace CSMWorld private: Class mClass; Type mType; - std::variant mValue; + std::variant mValue; friend bool operator==(const UniversalId& left, const UniversalId& right); diff --git a/apps/opencs_tests/model/world/testuniversalid.cpp b/apps/opencs_tests/model/world/testuniversalid.cpp index 93ddd6ce8f..960903c8de 100644 --- a/apps/opencs_tests/model/world/testuniversalid.cpp +++ b/apps/opencs_tests/model/world/testuniversalid.cpp @@ -73,6 +73,13 @@ namespace CSMWorld std::string mIcon; }; + std::ostream& operator<<(std::ostream& stream, const Params& value) + { + return stream << ".mType = " << value.mType << " .mClass = " << value.mClass + << " .mArgumentType = " << value.mArgumentType << " .mTypeName = " << value.mTypeName + << " .mString = " << value.mString << " .mIcon = " << value.mIcon; + } + struct CSMWorldUniversalIdValidPerTypeTest : TestWithParam { }; @@ -152,8 +159,11 @@ namespace CSMWorld UniversalId::ArgumentType_Id, "Instance", "Instance: f", ":./instance.png" }, Params{ UniversalId(UniversalId::Type_Reference, ESM::RefId::stringRefId("g")), UniversalId::Type_Reference, - UniversalId::Class_SubRecord, UniversalId::ArgumentType_Id, "Instance", "Instance: g", + UniversalId::Class_SubRecord, UniversalId::ArgumentType_RefId, "Instance", "Instance: \"g\"", ":./instance.png" }, + Params{ UniversalId(UniversalId::Type_Reference, ESM::RefId::index(ESM::REC_SKIL, 42)), + UniversalId::Type_Reference, UniversalId::Class_SubRecord, UniversalId::ArgumentType_RefId, "Instance", + "Instance: Index:SKIL:0x2a", ":./instance.png" }, }; INSTANTIATE_TEST_SUITE_P(ValidParams, CSMWorldUniversalIdValidPerTypeTest, ValuesIn(validParams)); From 7ba397da7d9f3a0a82414323a2ba1f79fc1218f6 Mon Sep 17 00:00:00 2001 From: elsid Date: Mon, 22 May 2023 02:36:01 +0200 Subject: [PATCH 5/8] Use std::span and std::string to define UniversalId related TypeData --- apps/opencs/model/world/universalid.cpp | 135 ++++++++++++------------ 1 file changed, 65 insertions(+), 70 deletions(-) diff --git a/apps/opencs/model/world/universalid.cpp b/apps/opencs/model/world/universalid.cpp index 5a74ca57af..8a6651b124 100644 --- a/apps/opencs/model/world/universalid.cpp +++ b/apps/opencs/model/world/universalid.cpp @@ -3,8 +3,10 @@ #include #include #include +#include #include #include +#include #include #include @@ -14,12 +16,12 @@ namespace { CSMWorld::UniversalId::Class mClass; CSMWorld::UniversalId::Type mType; - const char* mName; - const char* mIcon; + std::string_view mName; + std::string_view mIcon; }; - static const TypeData sNoArg[] = { - { CSMWorld::UniversalId::Class_None, CSMWorld::UniversalId::Type_None, "-", 0 }, + constexpr TypeData sNoArg[] = { + { CSMWorld::UniversalId::Class_None, CSMWorld::UniversalId::Type_None, "-", ":placeholder" }, { CSMWorld::UniversalId::Class_RecordList, CSMWorld::UniversalId::Type_Globals, "Global Variables", ":./global-variable.png" }, { CSMWorld::UniversalId::Class_RecordList, CSMWorld::UniversalId::Type_Gmsts, "Game Settings", ":./gmst.png" }, @@ -81,11 +83,9 @@ namespace ":./start-script.png" }, { CSMWorld::UniversalId::Class_RecordList, CSMWorld::UniversalId::Type_MetaDatas, "Metadata", ":./metadata.png" }, - // end marker - { CSMWorld::UniversalId::Class_None, CSMWorld::UniversalId::Type_None, 0, 0 }, }; - static const TypeData sIdArg[] = { + constexpr TypeData sIdArg[] = { { CSMWorld::UniversalId::Class_Record, CSMWorld::UniversalId::Type_Global, "Global Variable", ":./global-variable.png" }, { CSMWorld::UniversalId::Class_Record, CSMWorld::UniversalId::Type_Gmst, "Game Setting", ":./gmst.png" }, @@ -165,19 +165,15 @@ namespace { CSMWorld::UniversalId::Class_Record, CSMWorld::UniversalId::Type_StartScript, "Start Script", ":./start-script.png" }, { CSMWorld::UniversalId::Class_Record, CSMWorld::UniversalId::Type_MetaData, "Metadata", ":./metadata.png" }, - // end marker - { CSMWorld::UniversalId::Class_None, CSMWorld::UniversalId::Type_None, 0, 0 }, }; - static const TypeData sIndexArg[] = { + constexpr TypeData sIndexArg[] = { { CSMWorld::UniversalId::Class_Transient, CSMWorld::UniversalId::Type_VerificationResults, "Verification Results", ":./menu-verify.png" }, { CSMWorld::UniversalId::Class_Transient, CSMWorld::UniversalId::Type_LoadErrorLog, "Load Error Log", ":./error-log.png" }, { CSMWorld::UniversalId::Class_Transient, CSMWorld::UniversalId::Type_Search, "Global Search", ":./menu-search.png" }, - // end marker - { CSMWorld::UniversalId::Class_None, CSMWorld::UniversalId::Type_None, 0, 0 }, }; struct WriteToStream @@ -195,12 +191,12 @@ namespace struct GetTypeData { - const TypeData* operator()(std::monostate /*value*/) const { return sNoArg; } + std::span operator()(std::monostate /*value*/) const { return sNoArg; } - const TypeData* operator()(int /*value*/) const { return sIndexArg; } + std::span operator()(int /*value*/) const { return sIndexArg; } template - const TypeData* operator()(const T& /*value*/) const + std::span operator()(const T& /*value*/) const { return sIdArg; } @@ -216,20 +212,20 @@ CSMWorld::UniversalId::UniversalId(const std::string& universalId) { std::string type = universalId.substr(0, index); - for (int i = 0; sIdArg[i].mName; ++i) - if (type == sIdArg[i].mName) + for (const TypeData& value : sIdArg) + if (type == value.mName) { - mType = sIdArg[i].mType; - mClass = sIdArg[i].mClass; + mType = value.mType; + mClass = value.mClass; mValue = universalId.substr(index + 2); return; } - for (int i = 0; sIndexArg[i].mName; ++i) - if (type == sIndexArg[i].mName) + for (const TypeData& value : sIndexArg) + if (type == value.mName) { - mType = sIndexArg[i].mType; - mClass = sIndexArg[i].mClass; + mType = value.mType; + mClass = value.mClass; std::istringstream stream(universalId.substr(index + 2)); @@ -245,11 +241,11 @@ CSMWorld::UniversalId::UniversalId(const std::string& universalId) } else { - for (int i = 0; sNoArg[i].mName; ++i) - if (universalId == sNoArg[i].mName) + for (const TypeData& value : sIndexArg) + if (universalId == value.mName) { - mType = sNoArg[i].mType; - mClass = sNoArg[i].mClass; + mType = value.mType; + mClass = value.mClass; return; } } @@ -261,26 +257,26 @@ CSMWorld::UniversalId::UniversalId(Type type) : mType(type) , mValue(std::monostate{}) { - for (int i = 0; sNoArg[i].mName; ++i) - if (type == sNoArg[i].mType) + for (const TypeData& value : sNoArg) + if (type == value.mType) { - mClass = sNoArg[i].mClass; + mClass = value.mClass; return; } - for (int i = 0; sIdArg[i].mName; ++i) - if (type == sIdArg[i].mType) + for (const TypeData& value : sIdArg) + if (type == value.mType) { mValue = std::string(); - mClass = sIdArg[i].mClass; + mClass = value.mClass; return; } - for (int i = 0; sIndexArg[i].mName; ++i) - if (type == sIndexArg[i].mType) + for (const TypeData& value : sIndexArg) + if (type == value.mType) { mValue = int{}; - mClass = sIndexArg[i].mClass; + mClass = value.mClass; return; } @@ -291,10 +287,10 @@ CSMWorld::UniversalId::UniversalId(Type type, const std::string& id) : mType(type) , mValue(id) { - for (int i = 0; sIdArg[i].mName; ++i) - if (type == sIdArg[i].mType) + for (const TypeData& value : sIdArg) + if (type == value.mType) { - mClass = sIdArg[i].mClass; + mClass = value.mClass; return; } throw std::logic_error("invalid ID argument UniversalId type: " + std::to_string(type)); @@ -304,10 +300,10 @@ CSMWorld::UniversalId::UniversalId(Type type, ESM::RefId id) : mType(type) , mValue(id) { - for (int i = 0; sIdArg[i].mName; ++i) - if (type == sIdArg[i].mType) + for (const TypeData& value : sIdArg) + if (type == value.mType) { - mClass = sIdArg[i].mClass; + mClass = value.mClass; return; } throw std::logic_error("invalid RefId argument UniversalId type: " + std::to_string(type)); @@ -317,10 +313,10 @@ CSMWorld::UniversalId::UniversalId(Type type, int index) : mType(type) , mValue(index) { - for (int i = 0; sIndexArg[i].mName; ++i) - if (type == sIndexArg[i].mType) + for (const TypeData& value : sIndexArg) + if (type == value.mType) { - mClass = sIndexArg[i].mClass; + mClass = value.mClass; return; } @@ -360,11 +356,11 @@ int CSMWorld::UniversalId::getIndex() const std::string CSMWorld::UniversalId::getTypeName() const { - const TypeData* typeData = std::visit(GetTypeData{}, mValue); + const std::span typeData = std::visit(GetTypeData{}, mValue); - for (int i = 0; typeData[i].mName; ++i) - if (typeData[i].mType == mType) - return typeData[i].mName; + for (const TypeData& value : typeData) + if (value.mType == mType) + return std::string(value.mName); throw std::logic_error("failed to retrieve UniversalId type name"); } @@ -382,11 +378,11 @@ std::string CSMWorld::UniversalId::toString() const std::string CSMWorld::UniversalId::getIcon() const { - const TypeData* typeData = std::visit(GetTypeData{}, mValue); + const std::span typeData = std::visit(GetTypeData{}, mValue); - for (int i = 0; typeData[i].mName; ++i) - if (typeData[i].mType == mType) - return typeData[i].mIcon ? typeData[i].mIcon : ":placeholder"; + for (const TypeData& value : typeData) + if (value.mType == mType) + return std::string(value.mIcon); throw std::logic_error("failed to retrieve UniversalId type icon"); } @@ -395,9 +391,9 @@ std::vector CSMWorld::UniversalId::listReferenceabl { std::vector list; - for (int i = 0; sIdArg[i].mName; ++i) - if (sIdArg[i].mClass == Class_RefRecord) - list.push_back(sIdArg[i].mType); + for (const TypeData& value : sIdArg) + if (value.mClass == Class_RefRecord) + list.push_back(value.mType); return list; } @@ -406,31 +402,30 @@ std::vector CSMWorld::UniversalId::listTypes(int cl { std::vector list; - for (int i = 0; sNoArg[i].mName; ++i) - if (sNoArg[i].mClass & classes) - list.push_back(sNoArg[i].mType); + for (const TypeData& value : sNoArg) + if (value.mClass & classes) + list.push_back(value.mType); - for (int i = 0; sIdArg[i].mName; ++i) - if (sIdArg[i].mClass & classes) - list.push_back(sIdArg[i].mType); + for (const TypeData& value : sIdArg) + if (value.mClass & classes) + list.push_back(value.mType); - for (int i = 0; sIndexArg[i].mName; ++i) - if (sIndexArg[i].mClass & classes) - list.push_back(sIndexArg[i].mType); + for (const TypeData& value : sIndexArg) + if (value.mClass & classes) + list.push_back(value.mType); return list; } CSMWorld::UniversalId::Type CSMWorld::UniversalId::getParentType(Type type) { - for (int i = 0; sIdArg[i].mType; ++i) - if (type == sIdArg[i].mType) + for (const TypeData& value : sIdArg) + if (type == value.mType) { - if (sIdArg[i].mClass == Class_RefRecord) + if (value.mClass == Class_RefRecord) return Type_Referenceables; - if (sIdArg[i].mClass == Class_SubRecord || sIdArg[i].mClass == Class_Record - || sIdArg[i].mClass == Class_Resource) + if (value.mClass == Class_SubRecord || value.mClass == Class_Record || value.mClass == Class_Resource) { if (type == Type_Cell_Missing) return Type_Cells; From 292983d57ac742d51d377138f777d4fe49a7bee9 Mon Sep 17 00:00:00 2001 From: elsid Date: Mon, 22 May 2023 02:44:21 +0200 Subject: [PATCH 6/8] Show UniversalId value for all argument types in reports --- apps/opencs/model/tools/reportmodel.cpp | 15 ++++++++--- apps/opencs/model/world/universalid.cpp | 25 +++++++++++++++++++ apps/opencs/model/world/universalid.hpp | 2 ++ .../model/world/testuniversalid.cpp | 17 +++++++++++++ 4 files changed, 56 insertions(+), 3 deletions(-) diff --git a/apps/opencs/model/tools/reportmodel.cpp b/apps/opencs/model/tools/reportmodel.cpp index d8e99969a7..84a8c71f95 100644 --- a/apps/opencs/model/tools/reportmodel.cpp +++ b/apps/opencs/model/tools/reportmodel.cpp @@ -59,10 +59,19 @@ QVariant CSMTools::ReportModel::data(const QModelIndex& index, int role) const { CSMWorld::UniversalId id = mRows.at(index.row()).mId; - if (id.getArgumentType() == CSMWorld::UniversalId::ArgumentType_Id) - return QString::fromUtf8(id.getId().c_str()); + switch (id.getArgumentType()) + { + case CSMWorld::UniversalId::ArgumentType_None: + return QString("-"); + case CSMWorld::UniversalId::ArgumentType_Index: + return QString::number(id.getIndex()); + case CSMWorld::UniversalId::ArgumentType_Id: + return QString::fromStdString(id.getId()); + case CSMWorld::UniversalId::ArgumentType_RefId: + return QString::fromStdString(id.getRefId().toString()); + } - return QString("-"); + return QString("unsupported"); } case Column_Hint: diff --git a/apps/opencs/model/world/universalid.cpp b/apps/opencs/model/world/universalid.cpp index 8a6651b124..d7a820ad5d 100644 --- a/apps/opencs/model/world/universalid.cpp +++ b/apps/opencs/model/world/universalid.cpp @@ -201,6 +201,23 @@ namespace return sIdArg; } }; + + std::string toString(CSMWorld::UniversalId::ArgumentType value) + { + switch (value) + { + case CSMWorld::UniversalId::ArgumentType_None: + return "None"; + case CSMWorld::UniversalId::ArgumentType_Id: + return "Id"; + case CSMWorld::UniversalId::ArgumentType_Index: + return "Index"; + case CSMWorld::UniversalId::ArgumentType_RefId: + return "RefId"; + } + + return std::to_string(value); + } } CSMWorld::UniversalId::UniversalId(const std::string& universalId) @@ -354,6 +371,14 @@ int CSMWorld::UniversalId::getIndex() const throw std::logic_error("invalid access to index of non-index UniversalId"); } +ESM::RefId CSMWorld::UniversalId::getRefId() const +{ + if (const ESM::RefId* result = std::get_if(&mValue)) + return *result; + + throw std::logic_error("invalid access to RefId of " + ::toString(getArgumentType()) + " UniversalId"); +} + std::string CSMWorld::UniversalId::getTypeName() const { const std::span typeData = std::visit(GetTypeData{}, mValue); diff --git a/apps/opencs/model/world/universalid.hpp b/apps/opencs/model/world/universalid.hpp index 8ade978a5c..2d3385bcb4 100644 --- a/apps/opencs/model/world/universalid.hpp +++ b/apps/opencs/model/world/universalid.hpp @@ -170,6 +170,8 @@ namespace CSMWorld int getIndex() const; ///< Calling this function for a non-index type will throw an exception. + ESM::RefId getRefId() const; + std::string getTypeName() const; std::string toString() const; diff --git a/apps/opencs_tests/model/world/testuniversalid.cpp b/apps/opencs_tests/model/world/testuniversalid.cpp index 960903c8de..564d65a1ac 100644 --- a/apps/opencs_tests/model/world/testuniversalid.cpp +++ b/apps/opencs_tests/model/world/testuniversalid.cpp @@ -27,6 +27,11 @@ namespace CSMWorld EXPECT_THROW(UniversalId(UniversalId::Type_Activator, 42), std::logic_error); } + TEST(CSMWorldUniversalIdTest, shouldFailToConstructFromRefIdWithInvalidType) + { + EXPECT_THROW(UniversalId(UniversalId::Type_Search, ESM::RefId()), std::logic_error); + } + TEST(CSMWorldUniversalIdTest, shouldFailToConstructFromInvalidUniversalIdString) { EXPECT_THROW(UniversalId("invalid"), std::runtime_error); @@ -62,6 +67,18 @@ namespace CSMWorld EXPECT_EQ(id.getId(), "a"); } + TEST(CSMWorldUniversalIdTest, getRefIdShouldThrowExceptionForDefaultConstructed) + { + const UniversalId id; + EXPECT_THROW(id.getRefId(), std::logic_error); + } + + TEST(CSMWorldUniversalIdTest, getRefIdShouldReturnValueForConstructedFromRefId) + { + const UniversalId id(UniversalId::Type_Skill, ESM::IndexRefId(ESM::REC_SKIL, 42)); + EXPECT_EQ(id.getRefId(), ESM::IndexRefId(ESM::REC_SKIL, 42)); + } + struct Params { UniversalId mId; From 0aa569d4feecc15d34d22e9d0a57e019630b7060 Mon Sep 17 00:00:00 2001 From: elsid Date: Mon, 22 May 2023 02:54:32 +0200 Subject: [PATCH 7/8] Add UniversalId argument type to exception message on invalid access --- apps/opencs/model/world/universalid.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/opencs/model/world/universalid.cpp b/apps/opencs/model/world/universalid.cpp index d7a820ad5d..dec533b015 100644 --- a/apps/opencs/model/world/universalid.cpp +++ b/apps/opencs/model/world/universalid.cpp @@ -360,7 +360,7 @@ const std::string& CSMWorld::UniversalId::getId() const if (const std::string* result = std::get_if(&mValue)) return *result; - throw std::logic_error("invalid access to ID of non-ID UniversalId"); + throw std::logic_error("invalid access to ID of " + ::toString(getArgumentType()) + " UniversalId"); } int CSMWorld::UniversalId::getIndex() const @@ -368,7 +368,7 @@ int CSMWorld::UniversalId::getIndex() const if (const int* result = std::get_if(&mValue)) return *result; - throw std::logic_error("invalid access to index of non-index UniversalId"); + throw std::logic_error("invalid access to index of " + ::toString(getArgumentType()) + " UniversalId"); } ESM::RefId CSMWorld::UniversalId::getRefId() const From 63e01d86a3cb6eaf9499f3d7495899e2e9159ef3 Mon Sep 17 00:00:00 2001 From: elsid Date: Mon, 22 May 2023 03:09:19 +0200 Subject: [PATCH 8/8] Use string based UniversalId to check script for blacklist Blacklist is a vector of strings and isBlacklisted internally calls getId which throws exception for RefId based UniversalId. --- apps/opencs/model/tools/scriptcheck.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/opencs/model/tools/scriptcheck.cpp b/apps/opencs/model/tools/scriptcheck.cpp index b97f829e53..7a76efa483 100644 --- a/apps/opencs/model/tools/scriptcheck.cpp +++ b/apps/opencs/model/tools/scriptcheck.cpp @@ -106,7 +106,7 @@ void CSMTools::ScriptCheckStage::perform(int stage, CSMDoc::Messages& messages) mId = mDocument.getData().getScripts().getId(stage); - if (mDocument.isBlacklisted(CSMWorld::UniversalId(CSMWorld::UniversalId::Type_Script, mId))) + if (mDocument.isBlacklisted(CSMWorld::UniversalId(CSMWorld::UniversalId::Type_Script, mId.getRefIdString()))) return; // Skip "Base" records (setting!) and "Deleted" records