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.

Sign up for GitHub

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

Jump to bottom

add support for comparison with strong typedefs #658

Closed
wants to merge 1 commit into from
Closed

add support for comparison with strong typedefs #658

wants to merge 1 commit into from

Conversation

jbcoe
Copy link
Contributor

@jbcoe jbcoe commented May 12, 2016

closes #652

@philsquared
Copy link
Collaborator

philsquared commented May 12, 2016 •
edited
Loading

Thanks for this.
All looks reasonable, except for the enable_ifs and std::is_constructible - since I need to maintain support for C++98/ 03.
I might be able to come up with some raw SFINAE workarounds, although I'm little reluctant to use SFINAE at all as so many pre-C++11 compilers have patchy support for it.
The best option might be to conditionally compile the whole thing so that only C++11+ compilers see the templated version?

@jbcoe
Copy link
Contributor Author

jbcoe commented May 12, 2016

Sounds good to me. Do you have existing examples I can base an improved PR upon?

@jbcoe jbcoe force-pushed the n207-approx-and-strong-typedefs branch from a164579 to 22370db Compare May 13, 2016 09:45
@philsquared
Copy link
Collaborator

I'm not quite sure what you mean by existing examples. Specific to Approx? or in general?

Anyway, we happened to meet at lunch today (small world!) and you also mentioned that you wanted an overload of < (and >) on Approx - and this worried me a little. My main concern was that < would not always be the inverse of >. It also doesn't quite feel right to me to say, effectively: "Is this number less than that one (or just a little bit more - that's ok too)". I can't quite articulate why that feels so wrong - and it's probably just a misleading gut feel. I'm hoping others may be watching this thread who may have an opinion and want to weigh in?

@jbcoe
Copy link
Contributor Author

jbcoe commented May 13, 2016 •
edited
Loading

I've updated the PR so tests pass.

I wondered if you had existing macros to deal with conditional c++ 11 support

@jbcoe
Copy link
Contributor Author

jbcoe commented Jun 13, 2016

Are further changes to this PR required?

@jbcoe
Copy link
Contributor Author

jbcoe commented Sep 23, 2016

Are further changes needed to merge this?

I've been using my patched version of Catch without any issues for a few months now.

lightmare
lightmare reviewed Sep 23, 2016
View reviewed changes
include/internal/catch_approx.hpp
@@ -40,7 +44,19 @@ namespace Detail {
approx.scale( m_scale );
return approx;
}
#if defined(__cplusplus) && __cplusplus >= 201103L
template <typename T, typename = typename std::enable_if<std::is_constructible<T, double>::value>::type>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is enabled when T is constructible from double. But you actually do the inverse below. Should use std::is_constructible<double, T> here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well spotted. Fixed.

@jbcoe jbcoe force-pushed the n207-approx-and-strong-typedefs branch from e0f7964 to d3f930a Compare September 24, 2016 16:43
@jbcoe
Copy link
Contributor Author

jbcoe commented Oct 4, 2016

test failures are addressed by #716 and are unconnected with this change.

@philsquared
Copy link
Collaborator

Sorry I've been quiet on this. I'm gradually getting up to speed again now!

From my initial quick scan this looks good (with @lightmare's fix) but I now see what you mean about the conditional C++ macros. Catch #defines CATCH_CPP11_OR_GREATER for that purpose.

@jbcoe jbcoe force-pushed the n207-approx-and-strong-typedefs branch from e1356fe to 5a1c152 Compare October 4, 2016 16:25
lightmare
lightmare reviewed Oct 4, 2016
View reviewed changes
include/internal/catch_approx.hpp
@@ -44,7 +44,7 @@ namespace Detail {
approx.scale( m_scale );
return approx;
}
#if defined(__cplusplus) && __cplusplus >= 201103L
#if CATCH_CPP11_OR_GREATER
Copy link
Contributor

Choose a reason for hiding this comment

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

use #ifdef, it doesn't have a value

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not "Why you should prefer #ifs over #ifdefs" (somewhat past halfway) 💎

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Amended.

@jbcoe
Copy link
Contributor Author

jbcoe commented Oct 5, 2016

I'll push support for != too. Seems silly to leave this out.

@jbcoe jbcoe force-pushed the n207-approx-and-strong-typedefs branch from e421a85 to 5daacb7 Compare October 6, 2016 00:08
@jbcoe
Copy link
Contributor Author

jbcoe commented Oct 6, 2016

Squashed into one commit and ready to go.

@jbcoe jbcoe force-pushed the n207-approx-and-strong-typedefs branch from 5daacb7 to 5a9072d Compare January 9, 2017 13:50
@jbcoe
Copy link
Contributor Author

jbcoe commented Jan 9, 2017

I've rebased this again to handle the incorporation of <= and >= operators.
I chose to rebase to keep commit history clean.

@horenmar
Copy link
Member

This is now in master, I added a new feature toggle so that MSVC can use this as well (it still doesn't trigger the C++11 feature check).

@horenmar horenmar closed this Jan 26, 2017
@onqtam onqtam mentioned this pull request Apr 7, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinmoene martinmoene martinmoene left review comments

@lightmare lightmare lightmare left review comments

Assignees
No one assigned
Labels
Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support to Approx for comparison with constructible types
6 participants
@jbcoe @philsquared @horenmar @lightmare @martinmoene @krvajal