Update min/max to common implementation#680
Conversation
Walter Brown has given talks pointing out the design flaw in `std::min` and `std::max` (which Stepanov has acknowledged): in cases where they compare equal, `min` should return the first argument and `max` the second. We are already doing this for our hidden friend implementation inside `Quantity`, so now we do it for the rest of them. This also lets us get rid of the `using std::min` and `using std::max` declarations, and the corresponding tests. The reason we do this now, as part of the Eigen/vector/matrix push, is because `using_common_type` is going to become problematic, and we would like to simply delete it. This lets us get rid of users that exist outside of `quantity.hh`, which would be an awkward situation if there are no users _inside_ of `quantity.hh`. And it's not a hugely valuable utility anyway; it's easy to recreate the common unit/rep logic directly (as we now do). To help with these implementations, we add a new utility, too. `AppropriateCommonUnit<Q, Us...>` produces `CommonUnit<Us...>` when `Q` is `Quantity`, and `CommonPointUnit<Us...>` when `Q` is `QuantityPoint`. Since we didn't document the existing `AppropriateAssociatedUnit` utility, we don't document this either. Yes, it's in `au::`, but it's a very "in the weeds" utility that I'm ambivalent about advertising. Helps #484.
| using R = std::common_type_t<R1, R2>; | ||
| const auto qc1 = q1.template as<R>(U{}); | ||
| const auto qc2 = q2.template as<R>(U{}); | ||
| return (qc2 < qc1) ? qc1 : qc2; |
There was a problem hiding this comment.
Given this stable ordering is something we feel is a correctness issue, do we have a test for it?
There was a problem hiding this comment.
I added a bunch of tests. It's a very subtle issue. I decided to create a type with a label tag that doesn't participate in ordering. I had to add a huge number of ops just to get the type to compile as a rep, and even then, things like unit conversion didn't work. Maybe #52 will bring more clarity here, and we can simplify the test in the future (or not).
Anyway, even though I wasn't able to cover all overloads, we've got tests now, and I think the tests are good!
Interestingly, most of them already passed, I think because our hidden friend Quantity implementation already had this behavior. The failing test was one of the QuantityPoint ones, and it passes now.
There was a problem hiding this comment.
So I checked out some of Walter's presentations, and he suggests a common implementation for making this clear:
inline bool OutOfOrderByValue(... q1, ... q2) { return q2 < q1; }
/* min */ return OutOfOrderByValue(q1, q2) ? q2 : q1;
/* max */ return OutOfORderByValue(q1, q2) ? q1 : q2;Now, I don't know where I stand on the name. It's was a bit confusing at first, but the more I think about it and watch the talk, it's not too bad.
| using R = std::common_type_t<R1, R2>; | ||
| const auto qc1 = q1.template as<R>(U{}); | ||
| const auto qc2 = q2.template as<R>(U{}); | ||
| return (qc2 < qc1) ? qc1 : qc2; |
There was a problem hiding this comment.
So I checked out some of Walter's presentations, and he suggests a common implementation for making this clear:
inline bool OutOfOrderByValue(... q1, ... q2) { return q2 < q1; }
/* min */ return OutOfOrderByValue(q1, q2) ? q2 : q1;
/* max */ return OutOfORderByValue(q1, q2) ? q1 : q2;Now, I don't know where I stand on the name. It's was a bit confusing at first, but the more I think about it and watch the talk, it's not too bad.
|
|
||
| // Hidden friend overload (same unit, same rep). | ||
| TEST(Max, PrefersSecondArgForSameTypeQuantity) { | ||
| using LN = LabeledNumber<int>; |
There was a problem hiding this comment.
Style guide would indicate Ln:
| using LN = LabeledNumber<int>; | |
| using Ln = LabeledNumber<int>; |
2 participants
Walter Brown has given talks pointing out the design flaw in
std::minand
std::max(which Stepanov has acknowledged): in cases where theycompare equal,
minshould return the first argument andmaxthesecond. We are already doing this for our hidden friend implementation
inside
Quantity, so now we do it for the rest of them. This also letsus get rid of the
using std::minandusing std::maxdeclarations,and the corresponding tests.
The reason we do this now, as part of the Eigen/vector/matrix push, is
because
using_common_typeis going to become problematic, and we wouldlike to simply delete it. This lets us get rid of users that exist
outside of
quantity.hh, which would be an awkward situation if thereare no users inside of
quantity.hh. And it's not a hugely valuableutility anyway; it's easy to recreate the common unit/rep logic directly
(as we now do).
To help with these implementations, we add a new utility, too.
AppropriateCommonUnit<Q, Us...>producesCommonUnit<Us...>whenQis
Quantity, andCommonPointUnit<Us...>whenQisQuantityPoint.Since we didn't document the existing
AppropriateAssociatedUnitutility, we don't document this either. Yes, it's in
au::, but it's avery "in the weeds" utility that I'm ambivalent about advertising.
Helps #484.