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

toRadian(), toDegree, and mathematical constants #669

Merged
merged 6 commits into from
Apr 16, 2016
Merged

Conversation

jslee02
Copy link
Member

@jslee02 jslee02 commented Apr 15, 2016

This PR replaces the macros for mathematical constants with type safe template functions, and addes utility functions, toRadian(), toDegree(), for degree-radian conversion.

This resolves #314.

@mxgrey
Copy link
Member

mxgrey commented Apr 15, 2016

I like the idea of this pull request, although constants<T> could get somewhat annoying, especially since I think most of the time we will want to use constantsd anyway.

What if we also provided user-defined literals for pi and degrees? For example:

namespace dart {
namespace math {

constexpr double operator"" _pi (double x)
{
  return x * constantsd::pi();
}

constexpr double operator"" _deg(double x)
{
  return x * constantsd::pi() / 180.0;
}

} // namespace math
} // namespace dart

@jslee02
Copy link
Member Author

jslee02 commented Apr 16, 2016

I like the idea of this pull request, although constants could get somewhat annoying, especially since I think most of the time we will want to use constantsd anyway.

Right. I also considered providing only double version rather than templating. I don't have a strong preference here so I'm fine with that too.

Here are some other alternatives we could consider:

  • If we provide only double version, this template struct can be just namespace and each item would be variable instead of function as:
namespace constants {
constexpr double pi = 3.14xxx;
}

auto twoPi = 2.0 * constant::pi;  // not constant::pi()
  • We could consider shorter name like const::pi() or const::pi.

What if we also provided user-defined literals for pi and degrees?

Sounds really good. By the way, _deg and _rad seem to make sense to me, but I'm not sure if _pi is also good practice.

Sorry @mkoval @psigen, we're going to use another C++11 feature. 😈

@jslee02 jslee02 added this to the DART 6.0.0 milestone Apr 16, 2016
@mxgrey
Copy link
Member

mxgrey commented Apr 16, 2016

I'm happy with this pull request. I think we can merge once the CI tests pass. 👍

@jslee02 jslee02 merged commit 2eaf732 into master Apr 16, 2016
@jslee02 jslee02 deleted the feature/constants branch April 16, 2016 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use const variable instead of macro definitions for type safety
2 participants