-
Notifications
You must be signed in to change notification settings - Fork 793
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.
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
Scoped enums for ui_item.h #2468
Conversation
Apparently linking issues implies that this closes the issue, not just that it relates 😂 |
@@ -42,36 +43,60 @@ enum UiFlags : uint16_t { | |||
// clang-format on | |||
}; | |||
|
|||
inline UiFlags operator|(UiFlags lhs, UiFlags rhs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, thanks for pointing that out. (I think I actually read that comment trail and then remembered nothing from it when I was looking at this 😓 )
I was looking at C++20 constraints, which would provide for something like the following (if we could even target that version of the language):
template<typename T> requires std::is_enum_v<T>
inline T operator|(T lhs, T rhs)
{
using V = std::underlying_type_t<T>;
return static_cast<T>(static_cast<V>(lhs) | static_cast<V>(rhs));
}
Even if we could use C++20, C++ enums don't have any distinction between flags types and non-flags types like you get in other languages so that would allow for some nonsensical operations like Direction::DIR_S | Direction::DIR_N
.
The grisumbras/enum-flags library you found in that comment looks ideal, that wrapper class provides actual friend functions and a way to mark certain enums as being used for flags.
I think adding all the operator overloading is mostly considered a bad practice for |
Another point is that, IMHO, we should move away from flag enums and use |
ceca223
to
68e2d46
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good, but I'm not a fan of the flag functions being specific to UiFlags and not generally covers all enum based flags, but that might not be possible :/
68e2d46
to
47e930e
Compare
Not really possible without using C++20 constraints or a helper. The grisumbras/enum-flags lib @obligaron pointed out looks like a decent solution. I haven't had time to play with it yet but I'm aiming to give it a shot after testing Attempting to use the template function from #2468 (comment) without C++20 constraints leads to undefined behaviour as the function can be applied to non-enum types. Even with the use of constraints the best that can be done using C++ language features is that we allow the function to be applied to any enum type, not just enums that are intended for use as flags. |
Actually, I just tested template <typename ET, std::enable_if_t<std::is_enum_v<ET>, bool> = true>
constexpr ET operator|(ET lhs, ET rhs)
{
using T = std::underlying_type_t<ET>;
return static_cast<ET>(static_cast<T>(lhs) | static_cast<T>(rhs));
}
inline UiFlags operator|=(UiFlags &lhs, UiFlags rhs)
{
lhs = lhs | rhs;
return lhs;
} However this causes the operator| overload to be used for every enum | enum statement, even while declaring an enum... e.g. Line 160 in 8c18863
So limiting the scope becomes an issue. Could still do it as part of this PR if that's desirable. |
It would be interesting to see and i prefer thing not changing several times as that can lead to confusion regarding what is the preferred one. |
Had a bit of an experiment this evening that ended up taking long enough that I only got a couple of implementations working. Adding template specialisation to the overloads fits in reasonably well with the current usage. The macro enum-flags annoyingly takes a bit of work due to design decisions made by the author. The helpers from grisumbras/enum-flags#31 are needed so the macro can be used inside a namespace. The big issue with that library is it intentionally avoids creating values of the enum type that aren't represented by a single enum value, forcing the use of the wrapper any time you wish to store a combination of flags (or extracting the underlying value and casting to the enum type). This requires a whole bunch more boilerplate for cases like the Ui* classes to the point I haven't bothered with a fully compiling example. I haven't had a chance to investigate https://github.com/mrts/flag-set-cpp/blob/master/include/flag_set.hpp yet. It looks about as awkward as enum-flags so maybe it'll be largely the same if I get that working. |
Sounds like we should start by merging this pr as is 🙂 |
Replace operator&& with named function
Using optional arguments where possible so that all use cases can share the same constructor. Also moving member initialisation to the initialiser list syntax as this makes it clear that the derived class is only setting properties on that class, leaving the parent constructor to initialise members of the parent class.
Convention seems to be that scrollbar is treated as a single unhyphenated word so the UiScrollbar class was renamed to match.
Bit of an experiment to see how scoped enums read when used as a collection of flags, happy for any feedback. The operator overloads are mainly to replicate the implicit conversion to/from int that is allowed with unscoped enums.
Also providesoperator&&
as a test if a flag is set, though this does cause syntax errors if used in a conditional statement without parentheses to force correct precedence. This may be desirable :)