Add enchantum as an enum reflection backend#441
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @ZXShady, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
As gemini-code-assist, I've reviewed this pull request from ZXShady. The core intent of this PR is to replace the existing internal enum reflection implementation within reflect-cpp with an external library called enchantum. The author, who is also the author of enchantum, states that this change aims to reduce compile times, decrease maintenance burden on reflect-cpp for enum reflection, and provide more customization options through enchantum::enum_traits (like removing prefixes). The changes involve integrating the enchantum header and refactoring the internal enum handling logic to delegate to the new library.
Highlights
- Enum Reflection Backend Replacement: The primary change is the complete replacement of
reflect-cpp's custom enum reflection code with theenchantumlibrary. - Integration of
enchantum: Theenchantumlibrary is added as a third-party header file and integrated into the internal enum handling components. - Simplified Internal Logic: The internal code for getting enum names, converting between enums and strings, and determining enum properties (like being a flag enum) is significantly simplified by leveraging
enchantum's functionalities. - Enum Range Configuration: The existing
RFL_ENUM_RANGE_MINandRFL_ENUM_RANGE_MAXmacros are now used to configure the corresponding range macros within theenchantumlibrary.
Changelog
Click here to see the changelog
- include/rfl/internal/enums/Names.hpp
- Moved and redefined
RFL_ENUM_RANGE_MINandRFL_ENUM_RANGE_MAXmacros (lines 4-18). - Mapped
RFL_ENUM_RANGE_MINandRFL_ENUM_RANGE_MAXtoENCHANTUM_MIN_RANGEandENCHANTUM_MAX_RANGE(lines 20-28). - Removed old definitions of
RFL_ENUM_RANGE_MINandRFL_ENUM_RANGE_MAX(lines 44-45). - Changed the type of
enums_member toautofor potential type deduction improvements (line 87).
- Moved and redefined
- include/rfl/internal/enums/StringConverter.hpp
- Included the new
enchantum.hppheader (line 13). - Replaced the manual loop for
enum_to_single_stringwith a call toenchantum::to_string(lines 73-76). - Replaced the custom string-to-enum conversion logic in
single_string_to_enumwith a call toenchantum::cast(lines 87-95).
- Included the new
- include/rfl/internal/enums/get_enum_names.hpp
- Included the new
enchantum.hppheader (line 13). - Removed the old compiler-specific
get_enum_name_str_viewandget_enum_namefunctions (lines 44-79). - Removed the complex
NamesCombinerandCombineNamesmachinery (lines 108-160, 162-181). - Refactored
get_enum_namesto useenchantum::entriesand convert the result to the internalNamesstructure (lines 74-109). - Removed include for
is_scoped_enum.hpp(line 16).
- Included the new
- include/rfl/internal/enums/is_flag_enum.hpp
- Included the new
enchantum.hppheader (line 7). - Added a template specialization for
enchantum::is_bitflagbased on the presence of bitwise operators (lines 8-12). - Updated the
is_flag_enumconcept to useenchantum::BitFlagEnum(lines 18-20).
- Included the new
- include/rfl/internal/enums/is_scoped_enum.hpp
- Included the new
enchantum.hppheader (line 6). - Updated the
is_scoped_enumconcept to useenchantum::ScopedEnum(line 13).
- Included the new
- include/rfl/thirdparty/enchantum.hpp
- Added the complete
enchantumlibrary header file (lines 1-1495).
- Added the complete
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Enums stand so tall,
Names reflected, answering the call.
Code shrinks, compile times fall.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces enchantum as a new backend for enum reflection, which is a significant change. The goals of reducing compile times, decreasing maintenance burden on rfl's enum reflection, and enabling more customization are well-aligned with improving the library. The integration of enchantum, especially the way rfl communicates its flag enum definition via enchantum::is_bitflag specialization, appears thoughtfully done.
However, there are a few points to address, primarily concerning a potentially breaking change in default enum ranges and associated documentation updates. Additionally, some code cleanup in the integration and a minor point in the vendored enchantum.hpp are suggested.
Please also note that docs/enums.md (line 44) will need an update to reflect the new default enum range [-256, 256] instead of [0, 127]. Since this file is not part of the PR, this is just a reminder for a follow-up or inclusion in this PR.
Overall, this is a promising direction for enum reflection in rfl.
Summary of Findings
- Default Enum Range Change: The default values for
RFL_ENUM_RANGE_MINandRFL_ENUM_RANGE_MAXhave been changed. This is a potentially breaking change and the accompanying comments are outdated. - Code Cleanup: Commented-out code from a previous implementation exists in
get_enum_names.hppand could be removed. - Macro Definition in
enchantum.hpp: TheENCHANTUM_ASSERTmacro in the vendoredenchantum.hppdeclares variadic arguments that are not used in its expansion. - Documentation Update Needed (Outside PR Scope): The main
docs/enums.mdfile needs to be updated to reflect the new default enum ranges. (Noted as not commented due to settings, but mentioned in general feedback).
Merge Readiness
This pull request makes a significant and largely positive change by integrating enchantum for enum reflection. However, due to the potentially breaking change in default enum ranges and the outdated documentation for these macros, I recommend addressing these points before merging. The other suggestions are for improving code clarity and correctness within the newly added/modified files.
As an AI, I am not authorized to approve pull requests. Please ensure further review and approval from authorized maintainers after addressing the feedback.
| #ifndef ENCHANTUM_ASSERT | ||
| #include <cassert> | ||
| // clang-format off | ||
| #define ENCHANTUM_ASSERT(cond, msg, ...) assert(cond && msg) |
There was a problem hiding this comment.
The ENCHANTUM_ASSERT macro is defined as assert(cond && msg), but it takes a variadic argument ... that isn't used in the expansion.
If the ... is intended for future enhancements (e.g., formatted messages), perhaps a comment could clarify this. Otherwise, if it's not planned to be used, would simplifying the macro to #define ENCHANTUM_ASSERT(cond, msg) assert(cond && msg) be more straightforward?
ZXShady
force-pushed
the
enchantum_enum_reflection
branch
from
cf254d3 to
0832143
Compare
| // To change the default minimum range for all enum types, redefine the macro | ||
| // RFL_ENUM_RANGE_MIN. | ||
| #if !defined(RFL_ENUM_RANGE_MIN) | ||
| #define RFL_ENUM_RANGE_MIN -256 |
There was a problem hiding this comment.
Suggestion: The range (-256, 256) strikes me as rather odd, since it doesn't match the previous implementation, and also goes over the one-byte "boundary" of an underlying type. I haven't looked at your enchantum internals yet, but I suspect that this new > 9-bit enum range may have some performance implications.
There was a problem hiding this comment.
my library clamps the enum ranges so it doesn't matter.
I did a small test with magic enum, mine, and rfl in this gist
https://gist.github.com/ZXShady/74ebfe7d14ddcfeee6b0577a9ee4a396
time to compile
magic enum : 9 seconds
enchantum : 2.5 seconds
rfl : 19 seconds
and by performance you mean what exactly? compile time or runtime?
There was a problem hiding this comment.
Both compile time and also memory, but I suppose that would be driven by the underlying type more than the enum range?
There was a problem hiding this comment.
Both compile time and also memory, but I suppose that would be driven by the underlying type more than the enum range?
no the enum range determines the compile times. see my library readme
but if enum range exceeds underlying type min max it clamps the range.
the compile time are faster (see gist above) and memory usage should be the same, memory usage while compiling though is heavily reduced just like compile times
|
Unless the tests reveal some compiler/OS-specific problems we haven't discovered so far (unlikely), this is ready to be merged, IMO. @ZXShady , do you agree? @ZXShady , I have one more request: When you upload enchantum to vcpkg and Conan, please notify me. The maintainers of these package managers want to avoid us including thirdparty libraries that they also ship. If people have two different versions of the same library in their code and don't even know about it that can cause all sort of problems... |
I was thinking about using the split headers instead of the big one since I always keep forgetting to update it and it bloats compile times what do you think? I don't think you should merge now I think I will update the header file again since I am releasing version 0.1.0 today that removes some overloads.
I am thinking of writing a vcpkg port. |
|
@ZXShady , there appears to be one more issue on macOS: I am not entirely sure why this happening, because you are clearly including I am totally fine with splitting up the single header file. By the way, if you keep on forgetting to update it, you can write a simple Python script that does it for you and then have a GitHub Actions step automatically file a PR. |
|
@ZXShady , by the way, I am also taking a closer look at the build pipelines, specifically why they suddenly take so long. The issue appears to be that GitHub Actions fails to connect to the binary cache and then tries to do so over and over again. |
|
Github sctions seems to be failing for me as well right now. been waiting and no runner been picked up I split the headers and used the latest release you are free to merge if the tests succeed |
|
currently overriding enum_range doesn't allow prefix_length to be overriden as well. I think it would be better to also add a recommendation to instead override |
|
@ZXShady what exactly does the prefix length do? I think it can be easily added to the configurations... |
|
it removes N characters from the start of enum name. example in docs #include <enchantum/enchantum.hpp>
#include <enchantum/bitwise_operators.hpp>
// this is a bug enum it is outside of default range [-256,256] and it has this annoying prefix
enum BigEnumOutsideOfDefault : std::uint16_t {
BigEnumOutsideOfDefault_A =0,BigEnumOutsideOfDefault_B = 4096
};
template<>
struct enchantum::enum_traits<BigEnumOutsideOfDefault> {
constexpr static auto prefix_length = sizeof("BigEnumOutsideOfDefault_")-1;
constexpr static auto min = 0;
constexpr static auto max = 4096;
};
// prefix removed
static_assert(enchantum::names<BigEnumOutsideOfDefault>[0] == "A");
static_assert(enchantum::names<BigEnumOutsideOfDefault>[1] == "B");I anm myself against making aliases for existing things. if the user needs this he should just specialize enchantum's instead. |
|
@ZXShady I See. I suppose that is something we can always add later. |
|
@ZXShady the tests have finally succeeded. I don't why they take do long these days. But I am merging. Thank you so much for this great contribution. If you really manage to extend the range to -32,000 to 32,000, please open another PR. I would be very interested. |
The extended range will just require updating the header files btw. (I have actually implemented it currently for clang, it is still local though on my machine though, results are very promising) Thanks for accepting my contribution! |
3 participants
Disclaimer I am the author of enchantum.
This PR replaces reflect-cpp own enum reflection with enchantum's
enchantum::enum_traits(except unnamed unscoped enums) and not just scoped enums.