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

Use C++20 three-way comparison everywhere #164

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

tcbrindle
Copy link
Owner

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 returning bool if one element is "less than" other. With this PR, we now require that a comparator returns a value of type std::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 provide std::strong_order as a custom comparator to use the IEEE total order, or std::weak_order (which is the same, but treats positive and negative zero as equivalent), or std::weak_order_fallback to get the same behaviour as before if they're absolutely sure that the data does not contain NaNs.

Fixes #158

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
Copy link

codecov bot commented Jan 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (19e87a3) 97.99% compared to head (a010b3e) 98.06%.
Report is 61 commits behind head on main.

❗ Current head a010b3e differs from pull request most recent head 3b0438b. Consider uploading reports for the commit 3b0438b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #164      +/-   ##
==========================================
+ Coverage   97.99%   98.06%   +0.07%     
==========================================
  Files          66       69       +3     
  Lines        2392     2432      +40     
==========================================
+ Hits         2344     2385      +41     
+ Misses         48       47       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.
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.

Consistent comparisons
2 participants