Refactor XmlMapStorage enum handling to centralize type-to-string mappings and improve type safety while extending EnumIndexedArray with additional iteration utilities. - #508
Merged
Conversation
Enumerator TypeEnum::TypeEnum cannot have the same name as its enclosing enum TypeEnum
Reviewer's GuideRefactors XmlMapStorage enum type handling to be more type-safe and macro-driven, introducing EnumIndexedArray-based storage, compile-time sanity checks, and helper utilities for mapping between enum values, type tags, and strings, plus small enhancements to EnumIndexedArray iteration utilities. Class diagram for updated XmlMapStorage enum handlingclassDiagram
direction LR
class TypeEnum {
<<enum>>
RoomAlign
DoorFlag
ExitFlag
RoomLight
RoomLoadFlag
InfomarkClass
InfomarkType
RoomMobFlag
RoomPortable
RoomRidable
RoomSundeath
RoomTerrain
Type
}
class SanityCheckEnum {
<<enum>>
RoomAlign
DoorFlag
ExitFlag
RoomLight
RoomLoadFlag
InfomarkClass
InfomarkType
RoomMobFlag
RoomPortable
RoomRidable
RoomSundeath
RoomTerrain
Type
}
class EnumIndexedArray~T,E,SIZE~ {
- MMapper::Array~T,SIZE~ base
+ EnumIndexedArray()
+ T& at(E index)
+ const T& at(E index) const
+ T& operator[] (E index)
+ const T& operator[] (E index) const
+ size_t size() const
+ iterator begin()
+ iterator end()
+ const_iterator begin() const
+ const_iterator end() const
+ bool operator== (EnumIndexedArray other) const
+ bool operator!= (EnumIndexedArray other) const
+ void for_each(Callback callback)
+ void for_each(Callback callback) const
+ void for_each2(Callback callback)
+ void for_each2(Callback callback) const
}
class TypeEnumArray~T~ {
}
class Converter {
- EnumToStrings m_enumToStrings
- TypeEnumArray~QHash<QStringView,uint32_t>~ m_stringToEnums
+ Converter()
+ ~Converter()
+ void DELETE_CTORS_AND_ASSIGN_OPS()
+ template parseNumber~T~(QStringView str) : std::optional~T~
+ template fromString~ENUM~(QStringView str) : std::optional~ENUM~
+ template toString~ENUM~(ENUM val) : const char*
- std::optional~uint32_t~ stringToEnum(TypeEnum type, QStringView str) const
- const QString& enumToString(TypeEnum type, uint32_t val) const
}
class XmlMapStorage {
+ class Converter
}
TypeEnumArray~T~ --|> EnumIndexedArray~T,TypeEnum,NUM_XMLMAPSTORAGE_TYPE~
class EnumToStrings {
}
EnumToStrings --|> TypeEnumArray~std::vector<QString>~
Converter --> EnumToStrings : uses
Converter --> TypeEnumArray~QHash<QStringView,uint32_t>~ : uses
Converter ..> TypeEnum : indexes
Converter ..> EnumIndexedArray~T,E,SIZE~ : relies on
SanityCheckEnum ..> TypeEnum : compile_time_alignment
class Utilities {
+ bool isValid(TypeEnum type)
+ constexpr TypeEnum enumToType(RoomAlignEnum val)
+ constexpr TypeEnum enumToType(DoorFlagEnum val)
+ constexpr TypeEnum enumToType(ExitFlagEnum val)
+ constexpr TypeEnum enumToType(RoomLightEnum val)
+ constexpr TypeEnum enumToType(RoomLoadFlagEnum val)
+ constexpr TypeEnum enumToType(InfomarkClassEnum val)
+ constexpr TypeEnum enumToType(InfomarkTypeEnum val)
+ constexpr TypeEnum enumToType(RoomMobFlagEnum val)
+ constexpr TypeEnum enumToType(RoomPortableEnum val)
+ constexpr TypeEnum enumToType(RoomRidableEnum val)
+ constexpr TypeEnum enumToType(RoomSundeathEnum val)
+ constexpr TypeEnum enumToType(RoomTerrainEnum val)
+ constexpr TypeEnum enumToType(TypeEnum val)
+ constexpr const char* to_c_string(TypeEnum val)
+ EnumToStrings initEnumToStrings()
}
Utilities ..> TypeEnum
Utilities ..> EnumToStrings
Utilities ..> EnumIndexedArray~T,E,SIZE~
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In EnumIndexedArray::for_each2 the loop uses SIZE rather than the template parameter SIZE_ (as used elsewhere in the class), which will either fail to compile or pick up an unintended symbol; this should be changed to SIZE_ for consistency and correctness.
- EnumIndexedArray now has four similar traversal helpers (for_each/for_each2 const and non-const); consider reducing duplication by having the const overloads call the non-const versions or vice versa, and possibly choosing clearer names that distinguish between element-only and (enum, element) callbacks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In EnumIndexedArray::for_each2 the loop uses SIZE rather than the template parameter SIZE_ (as used elsewhere in the class), which will either fail to compile or pick up an unintended symbol; this should be changed to SIZE_ for consistency and correctness.
- EnumIndexedArray now has four similar traversal helpers (for_each/for_each2 const and non-const); consider reducing duplication by having the const overloads call the non-const versions or vice versa, and possibly choosing clearer names that distinguish between element-only and (enum, element) callbacks.
## Individual Comments
### Comment 1
<location path="src/mapstorage/XmlMapStorage.cpp" line_range="87-89" />
<code_context>
+
+// 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")
namespace { // anonymous
</code_context>
<issue_to_address>
**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:
```cpp
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):
```cpp
#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.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+87
to
+89
| #define TYPE_NAME_ENUM_VALUE(_x) (TypeEnum::_x) | ||
| #define TYPE_NAME_ENUM_TYPE(_x) _x##Enum | ||
| #define TYPE_NAME_C_STRING(_x) (#_x "Enum") |
There was a problem hiding this comment.
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_STRINGThis 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.
nschimme
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #508 +/- ##
==========================================
- Coverage 25.06% 25.06% -0.01%
==========================================
Files 510 510
Lines 42275 42281 +6
Branches 4574 4572 -2
==========================================
Hits 10596 10596
- Misses 31679 31685 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Refactor XmlMapStorage enum handling to centralize type-to-string mappings and improve type safety while extending EnumIndexedArray with additional iteration utilities.
Enhancements: