Skip to content

Refactor XmlMapStorage enum handling to centralize type-to-string mappings and improve type safety while extending EnumIndexedArray with additional iteration utilities. #508

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

Merged
nschimme merged 2 commits into from
Apr 8, 2026
Merged

Refactor XmlMapStorage enum handling to centralize type-to-string mappings and improve type safety while extending EnumIndexedArray with additional iteration utilities. #508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
85ddb53
refactor XmlMapStorage TypeEnum to fix warning and add sanity checks
nschimme Apr 3, 2026
c4b2f25
add EnumIndexedArray::for_each2 with 2-arg callback
nschimme Apr 3, 2026
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
23 changes: 23 additions & 0 deletions src/global/EnumIndexedArray.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,29 @@ class NODISCARD EnumIndexedArray : private MMapper::Array<T, SIZE_>
callback(x);
}
}
template<typename Callback>
void for_each(Callback &&callback) const
{
for (const auto &x : *this) {
callback(x);
}
}
template<typename Callback>
void for_each2(Callback &&callback)
{
for (size_t i = 0u; i < SIZE; ++i) {
const auto e = static_cast<E>(i);
callback(e, at(e));
}
}
template<typename Callback>
void for_each2(Callback &&callback) const
{
for (size_t i = 0u; i < SIZE; ++i) {
const auto e = static_cast<E>(i);
callback(e, at(e));
}
}

public:
NODISCARD bool operator==(const EnumIndexedArray &other) const
Expand Down
220 changes: 150 additions & 70 deletions src/mapstorage/XmlMapStorage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,22 +37,56 @@
#include <QXmlStreamReader>
#include <QXmlStreamWriter>

// clang-format off

// ---------------------------- XmlMapStorage::TypeEnum ------------------------
// list know enum types
//
// Each called X(_x) should use:
//
// TYPE_NAME_ENUM_VALUE(_x), or
// TYPE_NAME_ENUM_TYPE(_x) to construct the enum type name, or
// TYPE_NAME_STRING(_x) to construct the string representing the type name.
//
// Caution: these must match XFOREACH_CONVERTER defined below; the reason we can't just have
// a single mscro is because XFOREACH_CONVERTER calls this but macros can't be recursive.
#define XFOREACH_TYPE_ENUM(X) \
X(RoomAlignEnum) \
X(DoorFlagEnum) \
X(ExitFlagEnum) \
X(RoomLightEnum) \
X(RoomLoadFlagEnum) \
X(InfomarkClassEnum) \
X(InfomarkTypeEnum) \
X(RoomMobFlagEnum) \
X(RoomPortableEnum) \
X(RoomRidableEnum) \
X(RoomSundeathEnum) \
X(RoomTerrainEnum) \
X(TypeEnum)
X(RoomAlign) \
X(DoorFlag) \
X(ExitFlag) \
X(RoomLight) \
X(RoomLoadFlag) \
X(InfomarkClass) \
X(InfomarkType) \
X(RoomMobFlag) \
X(RoomPortable) \
X(RoomRidable) \
X(RoomSundeath) \
X(RoomTerrain) \
X(Type)

// Caution: these must match the enum types listed in XFOREACH_TYPE_ENUM above.
// X(_x, _xfor, _xdecl)
#define XFOREACH_CONVERTER(X, X_SEP) \
X(RoomAlign, XFOREACH_RoomAlignEnum, X_DECL_SINGLE) X_SEP() \
X(DoorFlag, XFOREACH_DOOR_FLAG, X_DECL_MULTI) X_SEP() \
X(ExitFlag, XFOREACH_EXIT_FLAG, X_DECL_MULTI) X_SEP() \
X(RoomLight, XFOREACH_RoomLightEnum, X_DECL_SINGLE) X_SEP() \
X(RoomLoadFlag, XFOREACH_ROOM_LOAD_FLAG, X_DECL_SINGLE) X_SEP() \
X(InfomarkClass, XFOREACH_INFOMARK_CLASS, X_DECL_SINGLE) X_SEP() \
X(InfomarkType, XFOREACH_INFOMARK_TYPE, X_DECL_SINGLE) X_SEP() \
X(RoomMobFlag, XFOREACH_ROOM_MOB_FLAG, X_DECL_SINGLE) X_SEP() \
X(RoomPortable, XFOREACH_RoomPortableEnum, X_DECL_SINGLE) X_SEP() \
X(RoomRidable, XFOREACH_RoomRidableEnum, X_DECL_SINGLE) X_SEP() \
X(RoomSundeath, XFOREACH_RoomSundeathEnum, X_DECL_SINGLE) X_SEP() \
X(RoomTerrain, XFOREACH_RoomTerrainEnum, X_DECL_SINGLE) X_SEP() \
X(Type, XFOREACH_TYPE_ENUM, X_DECL_TYPE_ENUM)

// clang-format on

#define TYPE_NAME_ENUM_VALUE(_x) (TypeEnum::_x)
#define TYPE_NAME_ENUM_TYPE(_x) _x##Enum
#define TYPE_NAME_C_STRING(_x) (#_x "Enum")
Comment on lines +87 to +89

@sourcery-ai sourcery-ai Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion (bug_risk): The helper TYPE_NAME_* macros are never undefined, which can inadvertently affect later code in this translation unit.

These helper macros are defined here but not undefined later in the file, unlike XFOREACH_TYPE_ENUM and XFOREACH_CONVERTER. This increases the risk of accidental reuse or name collisions elsewhere in the TU.

Please add #undef for TYPE_NAME_ENUM_VALUE, TYPE_NAME_ENUM_TYPE, and TYPE_NAME_C_STRING once they’re no longer needed (e.g., at the end of the file) to keep the macro namespace local.

Suggested implementation:

namespace { // anonymous

` section.

Here are the edit blocks:

<file_operations>
<file_operation operation="edit" file_path="src/mapstorage/XmlMapStorage.cpp">
<<<<<<< SEARCH
namespace { // anonymous

namespace { // anonymous

REPLACE
</file_operation>
</file_operations>

<additional_changes>
Please append the following block after the last use of the helper macros (typically near the end of XmlMapStorage.cpp, e.g., just before the final closing brace or before other unrelated includes/definitions):

#undef TYPE_NAME_ENUM_VALUE
#undef TYPE_NAME_ENUM_TYPE
#undef TYPE_NAME_C_STRING

This will ensure TYPE_NAME_ENUM_VALUE, TYPE_NAME_ENUM_TYPE, and TYPE_NAME_C_STRING are confined to this file’s local usage and cannot accidentally affect later code in the same translation unit. If the file has a clear “end-of-file” comment or a final function/namespace closing brace, place the #undef block immediately before that.


namespace { // anonymous

Expand All @@ -66,16 +100,104 @@ enum class NODISCARD TypeEnum : uint32_t {
constexpr const size_t NUM_XMLMAPSTORAGE_TYPE = (XFOREACH_TYPE_ENUM(X_ADD));
#undef X_ADD

NODISCARD bool isValid(const TypeEnum type)
{
return static_cast<uint32_t>(type) < NUM_XMLMAPSTORAGE_TYPE;
}

#define X_SEP() ,
#define X_DECL(_x, _xfor, _xdecl) _x
enum class NODISCARD SanityCheckEnum : uint32_t { XFOREACH_CONVERTER(X_DECL, X_SEP) };
#undef X_DECL
#undef X_SEP

#define X_SEP() ;
#define X_CHECK(_x, _xfor, _xdecl) \
static_assert(static_cast<uint32_t>(SanityCheckEnum::_x) \
== static_cast<uint32_t>(TYPE_NAME_ENUM_VALUE(_x)))
XFOREACH_CONVERTER(X_CHECK, X_SEP);
#undef X_CHECK
#undef X_SEP

#define X_SEP()
#define X_ADD(_x, _xfor, _xdecl) +1 // NOLINT
constexpr const size_t NUM_SANITYCHECK = (XFOREACH_CONVERTER(X_ADD, X_SEP));
#undef X_ADD
#undef X_SEP

static_assert(NUM_SANITYCHECK == NUM_XMLMAPSTORAGE_TYPE);

// define a bunch of methods
// static constexpr TypeEnum enumToType(RoomAlignEnum) { return TypeEnum::RoomAlign; }
// static constexpr TypeEnum enumToType(DoorFlagEnum) { return TypeEnum::DoorFlag; }
// ...
// converting an enumeration type to its corresponding TypeEnum value,
// which can be used as argument in enumToString() and stringToEnum()
#define X_DECL(_x) \
NODISCARD constexpr TypeEnum enumToType(TYPE_NAME_ENUM_TYPE(_x)) \
{ \
return TYPE_NAME_ENUM_VALUE(_x); \
}
XFOREACH_TYPE_ENUM(X_DECL)
#undef X_DECL

NODISCARD constexpr const char *to_c_string(const TypeEnum val)
{
#define X_CASE(_x) \
case TYPE_NAME_ENUM_VALUE(_x): \
return TYPE_NAME_C_STRING(_x);
//
switch (val) {
XFOREACH_TYPE_ENUM(X_CASE)
}
return "unknown";
#undef X_CASE
}

static_assert(enumToType(RoomAlignEnum::UNDEFINED) == TypeEnum::RoomAlign);
static_assert(enumToType(RoomTerrainEnum::UNDEFINED) == TypeEnum::RoomTerrain);

static_assert(enumToType(TypeEnum::RoomAlign) == TypeEnum::Type);
static_assert(enumToType(TypeEnum::RoomTerrain) == TypeEnum::Type);
static_assert(enumToType(TypeEnum::Type) == TypeEnum::Type);

static_assert(to_c_string(TypeEnum::RoomAlign) == std::string_view{"RoomAlignEnum"});
static_assert(to_c_string(TypeEnum::RoomTerrain) == std::string_view{"RoomTerrainEnum"});
static_assert(to_c_string(TypeEnum::Type) == std::string_view{"TypeEnum"});

template<typename T>
using TypeEnumArray = EnumIndexedArray<T, TypeEnum, NUM_XMLMAPSTORAGE_TYPE>;

using EnumToStrings = TypeEnumArray<std::vector<QString>>;

NODISCARD EnumToStrings initEnumToStrings()
{
#define X_SEP() ,
#define X_DECL_SINGLE(_x) /*QString*/ {#_x},
#define X_DECL_MULTI(_x, ...) /*QString*/ {#_x},
#define X_DECL_TYPE_ENUM(_x) /*QString*/ {TYPE_NAME_C_STRING(_x)},
#define X_CONVERT(_x, _xfor, _xdecl) (std::vector<QString>{_xfor(_xdecl)})

return EnumToStrings{XFOREACH_CONVERTER(X_CONVERT, X_SEP)};

#undef X_DECL_SINGLE
#undef X_DECL_MULTI
#undef X_DECL_TYPE_ENUM
#undef X_CONVERT
#undef X_SEP
}

// ---------------------------- XmlMapStorage::Converter -----------------------
class NODISCARD Converter final
{
private:
std::vector<std::vector<QString>> m_enumToStrings;
std::vector<QHash<QStringView, uint32_t>> m_stringToEnums;
EnumToStrings m_enumToStrings = initEnumToStrings();
TypeEnumArray<QHash<QStringView, uint32_t>> m_stringToEnums;

public:
Converter();
explicit Converter();
~Converter() = default;
DELETE_CTORS_AND_ASSIGN_OPS(Converter);

// parse string containing a signed or unsigned number.
template<typename T>
Expand All @@ -100,65 +222,26 @@ class NODISCARD Converter final
{
static_assert(std::is_enum_v<ENUM>, "template type ENUM must be an enumeration");
if constexpr (std::is_same_v<ENUM, TypeEnum>) {
#define X_CASE(x) \
case TypeEnum::x: \
return #x;
//
switch (val) {
XFOREACH_TYPE_ENUM(X_CASE)
}
return "unknown";
#undef X_CASE
return ::to_c_string(val);
} else {
return enumToString(enumToType(val), static_cast<uint32_t>(val));
}
}

private:
// define a bunch of methods
// static constexpr TypeEnum enumToType(RoomAlignEnum) { return TypeEnum::RoomAlignEnum; }
// static constexpr TypeEnum enumToType(DoorFlagEnum) { return TypeEnum::DoorFlagEnum; }
// ...
// converting an enumeration type to its corresponding Type value,
// which can be used as argument in enumToString() and stringToEnum()
#define X_DECL(X) \
MAYBE_UNUSED NODISCARD static constexpr TypeEnum enumToType(X) { return TypeEnum::X; }
XFOREACH_TYPE_ENUM(X_DECL)
#undef X_DECL

NODISCARD std::optional<uint32_t> stringToEnum(TypeEnum type, QStringView str) const;
NODISCARD const QString &enumToString(TypeEnum type, uint32_t val) const;
};

Converter::Converter()
: m_enumToStrings{
#define X_DECL(X) /* */ {#X},
#define X_DECL2(X, ...) {#X},
/* these must match the enum types listed in XFOREACH_TYPE_ENUM above */
{XFOREACH_RoomAlignEnum(X_DECL)},
{XFOREACH_DOOR_FLAG(X_DECL2)},
{XFOREACH_EXIT_FLAG(X_DECL2)},
{XFOREACH_RoomLightEnum(X_DECL)},
{XFOREACH_ROOM_LOAD_FLAG(X_DECL)},
{XFOREACH_INFOMARK_CLASS(X_DECL)},
{XFOREACH_INFOMARK_TYPE(X_DECL)},
{XFOREACH_ROOM_MOB_FLAG(X_DECL)},
{XFOREACH_RoomPortableEnum(X_DECL)},
{XFOREACH_RoomRidableEnum(X_DECL)},
{XFOREACH_RoomSundeathEnum(X_DECL)},
{XFOREACH_RoomTerrainEnum(X_DECL)},
{XFOREACH_TYPE_ENUM(X_DECL)},
#undef X_DECL
#undef X_DECL2
}
{
if (m_enumToStrings.size() != NUM_XMLMAPSTORAGE_TYPE) {
throw std::runtime_error("XmlMapStorage internal error: enum names do not match enum types");
}

// create the maps string -> enum value for each enum type listed above
for (auto &vec : m_enumToStrings) {
auto &map = m_stringToEnums.emplace_back();
m_enumToStrings.for_each2([this](const TypeEnum e, auto &vec) {
auto &map = m_stringToEnums[e];
uint32_t val = 0;
for (auto &str : vec) {
if (str == "UNDEFINED") {
Expand All @@ -172,22 +255,20 @@ Converter::Converter()
}
++val;
}
}
});
}

const QString &Converter::enumToString(const TypeEnum type, const uint32_t val) const
{
const auto index = static_cast<uint32_t>(type);
if (index < m_enumToStrings.size()) {
const auto &tmp = m_enumToStrings[index];
if (val < tmp.size()) {
if (isValid(type)) {
if (const auto &tmp = m_enumToStrings[type]; val < tmp.size()) {
return tmp[val];
}
}

qWarning().noquote().nospace()
<< "Attempt to save an invalid enum type = " << toString(type) << ", value = " << val
<< ". Either the current map is damaged, or there is a bug";
<< "WARNING: Attempt to save an invalid enum type = " << toString(type)
<< ", value = " << val << ". Either the current map is damaged, or there is a bug.";

static const QString g_empty{};
assert(g_empty.isEmpty());
Expand All @@ -196,11 +277,9 @@ const QString &Converter::enumToString(const TypeEnum type, const uint32_t val)

std::optional<uint32_t> Converter::stringToEnum(const TypeEnum type, const QStringView str) const
{
const auto index = static_cast<uint32_t>(type);
if (index < m_stringToEnums.size()) {
const auto &tmp = m_stringToEnums[index];
const auto iter = tmp.find(str);
if (iter != tmp.end()) {
if (isValid(type)) {
const auto &tmp = m_stringToEnums[type];
if (const auto iter = tmp.find(str); iter != tmp.end()) {
return iter.value();
}
}
Expand Down Expand Up @@ -950,3 +1029,4 @@ void XmlMapStorage::saveRoomMobFlags(QXmlStreamWriter &stream, const RoomMobFlag
}

#undef XFOREACH_TYPE_ENUM
#undef XFOREACH_CONVERTER
Loading