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

Add to_underlying_type to cast enum to underlying #1028

Closed
wants to merge 1 commit into from

Conversation

JoeLoser
Copy link
Contributor

Summary:

  • It is verbose at the call sites to cast an enum or enum class to its
    underlying type.
  • Introduce to_underlying_type, which, given an enum or enum class,
    returns the value from static casting to the underlying type.

Copy link
Contributor

@yfeldblum yfeldblum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks handy!

folly/Utility.h Outdated Show resolved Hide resolved
folly/Utility.h Outdated Show resolved Hide resolved
folly/Utility.h Outdated Show resolved Hide resolved
folly/Utility.h Outdated Show resolved Hide resolved
Summary:
- It is verbose at the call sites to cast an enum or enum class to its
  underlying type.
- Introduce `to_underlying_type`, which, given an enum or enum class,
  returns the value from static casting to the underlying type.
@JoeLoser
Copy link
Contributor Author

JoeLoser commented Feb 23, 2019

Thanks for the review, @yfeldblum. I've fixed up your comments.

What do you think about extending to_underlying_type return the value if a user passes in an integral type? This would make https://github.com/facebook/folly/blob/master/folly/DynamicConverter.h#L213 also eligible for to_underlying_type. Something like

template <class E>
constexpr std::underlying_type_t<E> to_underlying_type(E e) noexcept {
   static_assert(std::is_enum<E>::value || std::is_integral<E>::value, "not an enum or integral type");
   if (std::is_enum<E>::value)
   {
       return static_cast<std::underlying_type_t<E>>(e);
   }
   else if (std::is_integral<E>::value)
   {
        return e;
   }
 }

This would let callers use this utility in more generic programming contexts where you know you are working a regular value type, but not sure whether it is wrapped in an enum or enum class. It eliminates the need to do SFINAE machinery (such as checking if the type is an enum) to not force a hard error in a caller using to_underlying_type.

@yfeldblum
Copy link
Contributor

Maybe, but that sounds like a completely different function.

E.g. to_signed just wraps static_cast<make_signed_t<...>> (effectively).

Then you run into questions like, why not work on arithmetic but non-integral types, etc.?

std::underlying_type is only defined over complete enumeration type until C++20, and is only defined over complete types and only has a member type alias type for complete enumeration types since C++20. I'd just go with to_underlying_type being a wrapper for static_cast<underlying_type_t<...>> with param type deduction.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yfeldblum has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@JoeLoser JoeLoser deleted the to_underlying_type branch February 26, 2019 00:18
sandraiser pushed a commit to sandraiser/folly that referenced this pull request Mar 4, 2019
Summary:
- It is verbose at the call sites to cast an enum or enum class to its
  underlying type.
- Introduce `to_underlying_type`, which, given an enum or enum class,
  returns the value from static casting to the underlying type.
Pull Request resolved: facebook#1028

Reviewed By: stevegury

Differential Revision: D14199619

Pulled By: yfeldblum

fbshipit-source-id: fde85b9be50945a390b18ba1448a0137f3d7b63e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants