Skip to content
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

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

brianhou
Copy link
Contributor

@brianhou brianhou commented Feb 19, 2018

#339 might not be ideal because the "options" can be arbitrary ints, rather than combinations of the options that we actually enumerate. If that's a problem, maybe something like this would be better?

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 like if (mOptions & Options::POSITIONS). My intermediate workaround is to require Options::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

  • Document new methods and classes
  • Format code with make format

Before merging a pull request

  • Set version target by selecting a milestone on the right side
  • Summarize this change in CHANGELOG.md
  • Add unit test(s) for this change

@brianhou brianhou added the pr: discussion For discussion or proposals. If it passes to state of implementation, replace with WIP. label Feb 19, 2018
@codecov
Copy link

codecov bot commented Feb 20, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@c08d6a4). Click here to learn what that means.
The diff coverage is 72.72%.

@@            Coverage Diff            @@
##             master     #340   +/-   ##
=========================================
  Coverage          ?   77.62%           
=========================================
  Files             ?      261           
  Lines             ?     6592           
  Branches          ?        0           
=========================================
  Hits              ?     5117           
  Misses            ?     1475           
  Partials          ?        0
Impacted Files Coverage Δ
src/planner/WorldStateSaver.cpp 0% <0%> (ø)
src/statespace/dart/MetaSkeletonStateSaver.cpp 76% <100%> (ø)
include/aikido/common/EnumFlags.hpp 100% <100%> (ø)

@brianhou brianhou changed the base branch from enhancement/brianhou/saver-flags to master February 26, 2018 01:15
jslee02
jslee02 previously approved these changes Oct 13, 2018
Copy link
Member

@jslee02 jslee02 left a 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!

// clang-format off

#define AIKIDO_ENABLE_BITWISE_OPERATORS(X) \
template<> \
Copy link
Member

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)

/// Enable bitwise operators for strongly-typed enums.
///
/// Adapted from
/// http://blog.bitwigglers.org/using-enum-classes-as-type-safe-bitmasks/
Copy link
Member

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?

@jslee02 jslee02 added this to the Aikido 0.3.0 milestone Oct 14, 2018
@psigen
Copy link
Member

psigen commented Oct 18, 2018

Just curious @brianhou @jslee02, can you implement something like the following to make that explicit boolean cast work?

(See Update section)
https://stackoverflow.com/a/9883421

@aditya-vk aditya-vk modified the milestones: Aikido 0.3.0, Aikido 0.4.0 May 10, 2020
@brianhou brianhou modified the milestones: Aikido 0.4.0, Aikido 0.5.0 Aug 27, 2020
@aditya-vk aditya-vk removed their request for review September 17, 2021 03:56
@egordon egordon self-requested a review as a code owner September 17, 2021 21:06
@egordon
Copy link
Contributor

egordon commented Nov 23, 2022

Outdated, to consider for later

@egordon egordon closed this Nov 23, 2022
@egordon egordon reopened this Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: discussion For discussion or proposals. If it passes to state of implementation, replace with WIP.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants