-
Notifications
You must be signed in to change notification settings - Fork 30
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
Use strongly-typed enum flags. #340
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #340 +/- ##
=========================================
Coverage ? 77.62%
=========================================
Files ? 261
Lines ? 6592
Branches ? 0
=========================================
Hits ? 5117
Misses ? 1475
Partials ? 0
|
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.
Sorry for the late response. This looks great to me!
include/aikido/common/EnumFlags.hpp
Outdated
// clang-format off | ||
|
||
#define AIKIDO_ENABLE_BITWISE_OPERATORS(X) \ | ||
template<> \ |
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.
Nit: template <>
(a space between template
and the angle brackets)
include/aikido/common/EnumFlags.hpp
Outdated
/// Enable bitwise operators for strongly-typed enums. | ||
/// | ||
/// Adapted from | ||
/// http://blog.bitwigglers.org/using-enum-classes-as-type-safe-bitmasks/ |
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.
Nit: Maybe we want to make this as non-doxygen style comment because this comment is not specifically tied to a code but just a note?
Just curious @brianhou @jslee02, can you implement something like the following to make that explicit boolean cast work? (See Update section) |
Outdated, to consider for later |
This PR follows the blog post above. I think the biggest annoyance with this implementation is that there's no contextual conversion to
bool
, so you can't write something likeif (mOptions & Options::POSITIONS)
. My intermediate workaround is to requireOptions::NONE = 0
and compare directly to that.I'm not entirely convinced whether this complication is worth it. @jslee02, what do you think?
Before creating a pull request
make format
Before merging a pull request
CHANGELOG.md