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

Let Approx::epsilon also take float #888

Closed
tomvierjahn opened this issue Apr 11, 2017 · 2 comments
Closed

Let Approx::epsilon also take float #888

tomvierjahn opened this issue Apr 11, 2017 · 2 comments

Comments

@tomvierjahn
Copy link

tomvierjahn commented Apr 11, 2017

Description

  • Approx::Approx and its comparison operators can now take float
  • Adding this feature to Approx::epsilon would improve consistency and code

Somehow related to #873

Extra information

  • Catch version: v1.9.1 (Generated: 2017-04-09 21:21:06.285364)
@horenmar
Copy link
Member

horenmar commented Apr 24, 2017

I spent some time thinking about this, then suddenly got really busy and couldn't respond.

The reason why this wasn't a no-brainer change was that it isn't quite so simple as to add a float overload for the function, because that would trigger compile-time error for code like Approx(2).margin(2). The solution that we are using is to add a templated overload, that uses SFINAE to only work with types that can be converted to double.

Now, for Approx::margin this isn't semantically problematic, as Approx(temperature(72)).margin(temperature(2)), makes perfect sense. However for Approx::epsilon the situation is not quite so clear-cut, as Approx(temperature(72)).epsilon(temperature(0.001)) does not make semantic sense.

This could be solved in two ways.

  1. Specifically make Approx::epsilon only accept floating point types (using std::is_floating_point).
  2. Damn the semantics, make Approx::epsilon accept anything.

Personally I like the second option better.

There is also Approx::scale, but I don't think anybody is actually using that one.

@horenmar
Copy link
Member

After discussion with Phil, I decided to go with option 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants