-
Notifications
You must be signed in to change notification settings - Fork 31
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 C++20 three-way comparison everywhere #164
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also, don't #include <flux/core/numeric.hpp> in utils.hpp
Rather than using a boolean "less-than" comparator (defaulting to std::ranges::less), we'll use what C++20 gives us and require a comparator that returns `std::weak_ordering`, defaulting to std::compare_three_way. We'll also require that both arguments are the same type (ignoring cvref qualifiers), because I'd prefer explicit conversions rather than implicit conversions that mixed-type comparisons probably require. Finally, we'll add `flux::cmp::compare` as a default-constructed `std::compare_three_way` that can be passed as a comparator without needing to construct it.
Given an N-ary callable f (where N>=2), flip(f) returns an N-ary function object which, when invoked, calls f with the first two arguments swapped. So, for example, flip(f)(a, b) calls f(b, a), and flip(f)(a, b, c, d) calls f(b, a, c, d).
This is just flip(cmp::compare), but reads nicely when passed as a comparator
* Change strict_weak_order_for concept to use ordering_invocable<..., std::weak_ordering> rather than std::strict_weak_order * Change all default comparators from std::ranges::less to std::compare_three_way * Change all comparator usages from if(invoke(cmp, a, b)) to if (invoke(cmp, a, b) < 0) * Update custom comparators in tests/examples * in flux::sort, wrap the given comparator in a function object that just does std::is_lt(cmp(a, b)) rather than making lots of changes to pdqsort
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #164 +/- ##
=======================================
Coverage 98.30% 98.30%
=======================================
Files 70 70
Lines 2471 2479 +8
=======================================
+ Hits 2429 2437 +8
Misses 42 42 ☔ View full report in Codecov by Sentry. |
This is a nice bonus
There's still a lot of documentation that I actually need to *write*, of course...
These do the same as cmp::min/max, but accept arguments that are only partially_ordered, arbitrarily returning the first argument for min, or the second argument for max in the case where the arguments are unordered.
isaacy2012
reviewed
Feb 3, 2024
isaacy2012
reviewed
Feb 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
C++20 adds the "spaceship" operator which not only gives us three-way comparison, but also provides information about whether a type is strongly, weakly, or only partially ordered.
Many Flux algorithms require that a comparator models a strict weak ordering over the elements of a sequence. Up until now (with the exception of the
compare()
function) we have followed the STL/ranges model of having our comparators returningbool
if one element is "less than" other. With this PR, we now require that a comparator returns a value of typestd::weak_ordering
.The goal is to help users writing "proper" comparators; with a
bool
-returning function, it's very easy to accidentally write a comparator which is not a proper strict weak order. While this is still possible in the new model, it seems like it would be far less likely.One particular change is that it's no longer possible to sort a vector of floats or doubles using the default comparator, because floating point types are only partially ordered. This is by design. Floating point data that contains NaNs will break
sort()
and other functions that expect a weak ordering. With this change, users can now providestd::strong_order
as a custom comparator to use the IEEE total order, orstd::weak_order
(which is the same, but treats positive and negative zero as equivalent), orstd::weak_order_fallback
to get the same behaviour as before if they're absolutely sure that the data does not contain NaNs.Fixes #158