-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Classier enums #6760
Merged
Merged
Classier enums #6760
Conversation
This file contains 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
Merged
sakertooth
approved these changes
Jul 9, 2023
messmerd
reviewed
Jul 22, 2023
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.
Almost everything looks good to me. I just have a couple questions.
messmerd
approved these changes
Jul 23, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Converts most enums to enum classes. A few were left alone for one of the following reasons:
I also added an
lmms::Flags<T>
class template, which functions similarly toQFlags<T>
. In line with the conversion to enum classes, it is slightly stricter than its Qt equivalent in that it does not implicitly convert to an integer (but this can still be achieved with an explicit cast). There is a new macro in this header for declaring the appropriateoperator|
for the associated enum type - I'm not proud of it, but I tried and failed to achieve anything nearly as useable with templates.Other changes worth noting:
Count
. Any that were used numerically have had astd::size_t
constant with the original enumerator name added alongside the enum.Count
enumerators were removed instead.operator+(Octave, Note) -> int
. This enablesOctave
andNote
to be converted to enum classes without becoming unwieldy to use, and helps enforce correct usage of these types (i.e. nonsensical operations likeKey::F + Key::A
don't work).foo(static_cast<FooType>(9001))
.-Werror
.Formatting changes have been kept to a minimum, as this is already a large PR.