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

feat: zero support added #488

Closed
wants to merge 13 commits into from
Closed

feat: zero support added #488

wants to merge 13 commits into from

Conversation

mpusz
Copy link
Owner

@mpusz mpusz commented Aug 31, 2023

Resolves #487

@mpusz mpusz mentioned this pull request Aug 31, 2023
chiphogg
chiphogg previously approved these changes Aug 31, 2023
Copy link
Collaborator

@chiphogg chiphogg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

In my experience, the biggest use case for zero is comparison and sign checking. Initialization can also be really handy.

BTW, we later added an explicit = delete; for the zero constructor of quantity_point. I think that'd be nice here too.

@mpusz
Copy link
Owner Author

mpusz commented Sep 1, 2023

BTW, we later added an explicit = delete; for the zero constructor of quantity_point. I think that'd be nice here too.

I don't think it is needed as in mp-units we do not have a constructor in quantity_point that gets a quantity as an argument. The only way to create a point is to add quantity to a point origin or use make_quantity_point() factory function.

@chiphogg
Copy link
Collaborator

chiphogg commented Sep 1, 2023

BTW, we later added an explicit = delete; for the zero constructor of quantity_point. I think that'd be nice here too.

I don't think it is needed as in mp-units we do not have a constructor in quantity_point that gets a quantity as an argument. The only way to create a point is to add quantity to a point origin or use make_quantity_point() factory function.

My main motivation for adding it was to communicate clearly to users that yes, we thought about this and no, we don't want to support it. I was anticipating that users who get "spoiled" by using zero with quantity will be tempted to use it with quantity point. So I decided to add the = delete; along with a readable comment for users who follow the delete to its source.

I guess I'm trying to head off a misguided feature request before it can happen. 🙂

@JohelEGP
Copy link
Collaborator

JohelEGP commented Sep 1, 2023

So I decided to add the = delete; along with a readable comment for users who follow the delete to its source.

It's more readable if it's a line comment after = delete;,
as Clang and GCC will include parts of it in the error message.
For example, the error of https://cpp2.godbolt.org/z/3fcosGbc4 includes:

main.cpp2:4:8: error: call to deleted constructor of 't'
    4 |   auto y {x}; 
      |        ^ ~~~
main.cpp2:3:15: note: 't' has been explicitly marked deleted here
    3 |       public: t(t const&) = delete; /* No 'that' constructor, suppress copy */
      |               ^
1 error generated.

@mpusz
Copy link
Owner Author

mpusz commented Sep 1, 2023

I think I would add it as well if we had a constructor from quantity in quantity_point. In our case, the only misleading thing here may be trying to use it as a point origin. For example, zero + 21 * deg_C is a quantity and not a quantity_point.

@mpusz
Copy link
Owner Author

mpusz commented Sep 1, 2023

Unfortunately, we have to get rid of some interfaces for zero so it will be limited in scope. operator-(QuantityPoint, QuantityPoint) can't return zero 😢

@chiphogg
Copy link
Collaborator

chiphogg commented Sep 2, 2023

Unfortunately, we have to get rid of some interfaces for zero so it will be limited in scope. operator-(QuantityPoint, QuantityPoint) can't return zero 😢

Hmm, that's too bad. Then again, it's probably just fine to start small and deliver value in a few use cases, then expand later as we're able.

By the way, there's one more non-obvious use case for zero I thought worth mentioning. It turns out that at least for Au, zero is a really useful result type for the origin displacement between two origins whose values are equal. If we didn't do this, we wouldn't be able to get the underlying value of a quantity point in its own unit! (At least, not without some other workaround.) In case you encounter this problem in mp-units, this could be useful to have in your bag of tricks.

https://github.com/aurora-opensource/au/blob/eb615a60a0588b259b3f341279845249ce7cfd09/au/unit_of_measure.hh#L416-L435

@mpusz
Copy link
Owner Author

mpusz commented Sep 2, 2023

Hmm, that's too bad. Then again, it's probably just fine to start small and deliver value in a few use cases, then expand later as we're able.

I do not think it will be possible. For example the below would compile:

QuantityOf<isq::energy> auto q = isq::width(2 * m) * (mean_sea_level - mean_sea_level);

which would be really problematic, right?

It is exactly why I removed zero being the result of subtracting same origins in this commit: 0dd28c1.

@chiphogg
Copy link
Collaborator

chiphogg commented Sep 2, 2023

I do not think it will be possible. For example the below would compile:

QuantityOf<isq::energy> auto q = isq::width(2 * m) * (mean_sea_level - mean_sea_level);

which would be really problematic, right?

It is exactly why I removed zero being the result of subtracting same origins in this commit: 0dd28c1.

If we made the result of subtracting an origin from itself zero --- and, if we did not define multiplication operators for zero --- then wouldn't this example not compile? Specifically, width(...) * zero wouldn't compile (even before we get to the LHS of the assignment).

@mpusz
Copy link
Owner Author

mpusz commented Sep 2, 2023

If we made the result of subtracting an origin from itself zero --- and, if we did not define multiplication operators for zero --- then wouldn't this example not compile? Specifically, width(...) * zero wouldn't compile (even before we get to the LHS of the assignment).

Yes, you are right, but I think making it not compile would be wrong and inconsistent. Why the first one should not compile while others compile fine:

auto q1 = isq::width(2 * m) * (mean_sea_level - mean_sea_level);
auto q2 = isq::width(2 * m) * (ground_level - mean_sea_level);
auto q3 = isq::width(2 * m) * (my_quantity_point - mean_sea_level);
auto q3 = isq::width(2 * m) * (my_quantity_point1 - my_quantity_point2);

Fixed the code after the comment below

What if I have a generic function like this:

constexpr auto point_diff(const QuantityPointOrPointOrigin& lhs, const QuantityPointOrPointOrigin& rhs)
{
  return lhs - rhs;
}

or something similar. How do we explain to the user that sometimes a result from such a function would make the following code not compile?

@mpusz
Copy link
Owner Author

mpusz commented Sep 2, 2023

To summarize, I think that if we decide that the subtraction of two points (that typically returns a quantity) in some specific case returns zero, then such zero should be handled by all the code that would accept quantity (including satisfying a Quantity concept). Such a solution would start a waterfall of other issues in the code... 🥴

@chiphogg
Copy link
Collaborator

chiphogg commented Sep 2, 2023

Ah --- so in #488 (comment), you meant to say that it would not compile, and that this is what would be problematic. Now I understand the argument. It's a fair point.

(Also, I think you're assigning an area to an energy in all of these examples, but that's a side point. 🙂)

This is an analogous problem to the decision to return a raw number when units cancel completely. I think mp-units used to do that, but then stopped. Au did not use to do that, and we now do --- here's the rationale --- but I'm starting to second-guess that decision. If I could go back in time, I'd probably advise my past self to prefer consistency and just always return a quantity.

So I'm persuaded to avoid returning zero in this new case, too.

friend consteval zero_t operator%(zero_t, zero_t) = delete;

[[nodiscard]] consteval bool operator==(zero_t) const { return true; }
[[nodiscard]] consteval auto operator<=>(zero_t) const { return std::partial_ordering::equivalent; }
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 std::strong_ordering?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Initially, I thought that the following is the reason for this:

static_assert(zero == zero);
static_assert(!(zero != zero));
static_assert(!(zero < zero));
static_assert(!(zero > zero));

but you are right. I see now that I was wrong. Thanks!

@mpusz
Copy link
Owner Author

mpusz commented Sep 2, 2023

(Also, I think you're assigning an area to an energy in all of these examples, but that's a side point. 🙂)

Fixed the code. Thanks for pointing this out.

@mpusz
Copy link
Owner Author

mpusz commented Sep 2, 2023

Ah --- so in #488 (comment), you meant to say that it would not compile, and that this is what would be problematic. Now I understand the argument. It's a fair point.

I actually meant both. There is no good solution for that. Either you make it not compile which makes it inconsistent or you make it compile and suddenly zero has to actually satisfy Quantity with all the side effects of such a change which is unfortunate because something that should be a quantity of area would also be able to initialize totally unrelated quantity (i.e. energy).

@JohelEGP
Copy link
Collaborator

JohelEGP commented Sep 2, 2023

Ah --- so in #488 (comment), you meant to say that it would not compile, and that this is what would be problematic. Now I understand the argument. It's a fair point.

I actually meant both. There is no good solution for that. Either you make it not compile which makes it inconsistent or you make it compile and suddenly zero has to actually satisfy Quantity with all the side effects of such a change which is unfortunate because something that should be a quantity of area would also be able to initialize totally unrelated quantity (i.e. energy).

Besides the current zero,
a zero that is parameterized on the dimension could solve that.

@mpusz
Copy link
Owner Author

mpusz commented Sep 2, 2023

Besides the current zero, a zero that is parameterized on the dimension could solve that.

I think that would be an overkill of a feature. In order to use it must be simple and universal. It should be usable not only with physical units but also with other numeric libraries.

Today, I sent a question to the SG6 (Numerics) chair, asking if he finds it useful in other use cases. If he is not interested, I am not sure if we will merge it as I see too many issues with this solution compared to the benefits of having it.

@chiphogg
Copy link
Collaborator

chiphogg commented Sep 4, 2023

I agree: dimension-based zero would be overkill.

I'm surprised we might not merge it if the SG6 chair doesn't already know of other use cases. I think the units library use cases are enough to justify it. It's true that we expect zero_t to be useful in other non-units use cases, too, but I wouldn't be surprised if people didn't see the value until they've had a chance to try it out --- I myself underestimated it until I started using it in practice.

For the units library use cases specifically, I think it makes sense to start conservatively with "whatever we can get by making quantity implicitly constructible from zero_t". This avoids the waterfall of code changes, while delivering on the most compelling use cases, which are initialization and comparison.

I also think it makes sense to think of zero as mostly something we enable end users to write, instead of something we try to look for opportunities to return in our APIs.

Ultimately, though, the decision to include or exclude zero_t is yours.

@mpusz
Copy link
Owner Author

mpusz commented Sep 4, 2023

I'm surprised we might not merge it if the SG6 chair doesn't already know of other use cases. I think the units library use cases are enough to justify it.

I am really puzzled here. It is much easier to add it later (especially as it is already implemented on the branch) when we find it mandatory than remove it later when people will actually start using it, but when we decide it was not a good idea.

I see some use cases where it helps, but I also see some cases where it actually causes lots of confusion. Even such an experienced user with this feature as you, suggested using it for the subtraction of two points because it really feels like it is the right way to do it. But it is not...

And there are more possible confusions coming. I am afraid of GitHub issues asking why the following does not work:

msl_altitude current_ = mean_sea_level + zero 

or this one:

for (auto tt = zero; tt <= 50 * ms; ++tt) { 
  // ...
}

or this:

auto q1 = 42 * m;
auto q2 = q1 * zero;

All of the above looks like the code that "should" work with a feature zero, and inexperienced users will be making those mistakes and asking for support when it does not compile.

I think it makes sense to start conservatively with "whatever we can get by making quantity implicitly constructible from zero_t". This avoids the #488 (comment), while delivering on the most compelling use cases, which are initialization and comparison.

Again, despite your long experience with the feature (please don't treat it personally; I just want to make a point here that even experts might be easily confused here), even the above use case has some issues connected with it:

void foo(quantity<si::metre> q);
void boo(QuantityOf<isq::length> auto q);

foo(zero); // works
boo(zero); // does not work

And what if someone will be already using zero in the core base and then the interface will be refactored from foo() to boo()?

Another similar issue with the above-proposed design:

auto q1 = 42 * m;
q1 += zero;          // works
auto q2 = q1 + zero; // does not work

Initially, I thought that zero is a great idea, but the more experience I get with this feature, the more issues like the above I see. I am sure there plenty more that I do not realize as of now as well...

As I said, there are certainly some cases where it makes the code nicer or more generic, but we may fall into plenty of possible misuses and pitfalls if we add it.

Let's remember that this feature is mostly to write this:

if (v1 - v2 == zero) ...

instead of this:

if (v1 - v2 == 0 * (m/s)) ...

I am not so convinced that it is worth it...

@chiphogg
Copy link
Collaborator

chiphogg commented Sep 4, 2023

tl;dr: Thanks for collecting these problem cases in one place! I looked at each, and I didn't find most of them compelling, because I don't believe users would be motivated to write them in the first place. However, I do think the issue of passing zero to a concept interface is both novel (this doesn't happen in Au) and fascinating, and I don't know the implications. Overall, if you don't feel confident adding it and decide to delay, then I'll understand your reasons.


I am really puzzled here. It is much easier to add it later (especially as it is already implemented on the branch) when we find it mandatory than remove it later when people will actually start using it, but when we decide it was not a good idea.

That's true. It's always easier to add later than to remove.

I see some use cases where it helps, but I also see some cases where it actually causes lots of confusion. Even such an experienced user with this feature as you, suggested using it for the subtraction of two points because it really feels like it is the right way to do it. But it is not...

Which comment are you referring to? Was it this one or some other?

Assuming that's the one, I think it was an oversight on my part that I didn't question the premise of your comment: namely, that we would want operator-(QuantityPoint, QuantityPoint) to be able to return zero. That's not what Au does, and I don't think it's desirable generally. When I said "it's too bad" that it wouldn't, I wrote without thinking.

What the rest of that comment refers to is an encapsulated, library-internal implementation detail. There's a use case when computing origin offsets where we want to use zero instead of the result of operator-(QuantityPoint, QuantityPoint). That use case might also be relevant for mp-units, although the quantity point interfaces and implementations are different enough that I really couldn't say for sure.

And there are more possible confusions coming. I am afraid of GitHub issues asking why the following does not work:

Thanks for supplying all of these concrete examples in one place! I'll look at each one in turn, to see how much it motivates me to avoid or defer adding zero.

msl_altitude current_ = mean_sea_level + zero 

I'm not fluent with mp-units constructs, so I'll write out my assumptions explicitly. I assume:

  • msl_altitude is a quantity point type, whose origin type is mean_sea_level
  • mean_sea_level is a type that represents a point origin: it represents a point only, and, crucially, has no associated units (although it presumably does have a dimension)

If those assumptions are correct, then mean_sea_level + zero indeed won't compile, because mean_sea_level has no units. This makes sense.

My question: would we expect this to work without the + zero? I would assume yes, so I would assume there's no motivation to reach for zero in the first place.

or this one:

for (auto tt = zero; tt <= 50 * ms; ++tt) { 
  // ...
}

Here's another use case where I just don't think users are motivated to reach for zero in the first place. If the unit is easy to write in the second clause, then it's easy enough to write in the first clause.

I think your main point, though, is that if a user does try this, then we might get an annoying bug report. I don't think we need to worry about this. The compiler error will mention operator++(zero_t). It should be straightforward enough that one can't increment a type which is always zero.

In fact, I think the situation is analogous to something we already have with an existing standard type that represents a kind of "zero-ness":

auto foo_ptr = nullptr;
++foo_ptr;

Next issue:

or this:

auto q1 = 42 * m;
auto q2 = q1 * zero;

I concede that if zero exists, it's tempting to add definitions for its multiplicative properties, not just its additive ones. That said, I think this would be a mistake (not that you were proposing this!).

It's interesting to contrast the additive and multiplicative properties for zero and 0 in the context of quantities:

  • For additive operations, 0 simply won't work, whereas zero will, and the result type is perfectly clear.
  • For multiplicative operations, 0 already works just fine, whereas the desired result from using zero is completely unclear: we don't even know the desired dimension!

So, yes: zero may not work for multiplicative use cases, but it's also true that because 0 works great here, users aren't motivated to reach for zero in the first place. Thus, I'm not really bothered by this use case.

All of the above looks like the code that "should" work with a feature zero, and inexperienced users will be making those mistakes and asking for support when it does not compile.

I think it makes sense to start conservatively with "whatever we can get by making quantity implicitly constructible from zero_t". This avoids the #488 (comment), while delivering on the most compelling use cases, which are initialization and comparison.

Again, despite your long experience with the feature (please don't treat it personally; I just want to make a point here that even experts might be easily confused here), even the above use case has some issues connected with it:

Thanks for clarifying that it's not meant personally. 🙂 And don't worry, I haven't taken anything personally, and I understand the point you're making by referencing my experience. In fact, I think the below use case is really interesting.

void foo(quantity<si::metre> q);
void boo(QuantityOf<isq::length> auto q);

foo(zero); // works
boo(zero); // does not work

And what if someone will be already using zero in the core base and then the interface will be refactored from foo() to boo()?

I think the reason the "implicitly convertible" use cases are such a slam dunk for Au, but have questionable use cases like this for mp-units, is at least in part due to concepts. Au virtually always deals in concrete types --- and as long as zero knows what it is "impersonating", it's really smooth. But bring in something like QuantityOf<isq::length>, and you're right, we see some friction.

I don't have a fully satisfying general answer. In this specific case, if one refactored an interface like this, and if there are many existing callsites using zero, it may be worth adding a zero_t overload either temporarily or permanently. This won't scale well, though, if we have multiple quantity parameters in a signature that we're refactoring to use concepts. It's then tempting to treat zero as satisfying the concept, as suggested elsewhere, but I think this would be a big mistake (which I think was the conclusion that we reached there too).

The big takeaway, though, is that "zero has issues with quantity concepts". That's novel and interesting, and I'm not sure what the full implications are just yet.

Another similar issue with the above-proposed design:

auto q1 = 42 * m;
q1 += zero;          // works
auto q2 = q1 + zero; // does not work

I'm confused: why wouldn't this work? I assume q2 should have the same type and value as q1. I just did a sanity check in Au, and indeed this works:

TEST(Zero, CanAddToQuantity) {
    auto q1 = meters(42);
    auto q2 = q1 + ZERO;
    EXPECT_THAT(q2, SameTypeAndValue(q1));
}

The test passes.

That said, even if it didn't, why would users be motivated to add zero to the quantity?

Initially, I thought that zero is a great idea, but the more experience I get with this feature, the more issues like the above I see. I am sure there plenty more that I do not realize as of now as well...

As I said, there are certainly some cases where it makes the code nicer or more generic, but we may fall into plenty of possible misuses and pitfalls if we add it.

I'm persuaded by the argument that it's easier to add than to remove, so if you're feeling uncomfortable about it, then I support your decision to defer.

Let's remember that this feature is mostly to write this:

if (v1 - v2 == zero) ...

instead of this:

if (v1 - v2 == 0 * (m/s)) ...

I am not so convinced that it is worth it...

Well, what are v1 and v2, exactly? Are they both of type (m/s)? Let's zoom out a bit.

bool equal(QuantityOf<isq::speed> auto v1, QuantityOf<isq::speed> auto v2) {
    return (v1 - v2 == 0 * (m/s));
}

In this case, we may get an extra unit conversion at runtime, between (m/s) and the common unit of v1 and v2. zero would pick the correct unit automatically. (Of course, we have to ignore how contrived the example is, as in real life we'd just write v1 == v2. 🙂)

It's also really interesting that in this case, concepts actually strengthen the arguments for zero. It seems like zero is questionable when passed to an interface using concepts, but it's still really handy inside of the implementation.

@mpusz
Copy link
Owner Author

mpusz commented Sep 5, 2023

Thanks for the detailed comments and answers!

Here are some comments from me:

Which comment are you referring to? Was it #488 or some other?

Yes, this is the one I meant.

My question: would we expect this to work without the + zero? I would assume yes, so I would assume there's no motivation to reach for zero in the first place.

Everything depends on the use case. Let's imagine this:

msl_altitude make_AMSL(auto q)
{
  return mean_sea_level + q;
}

msl_altitude qp1 = make_AMSL(42 * m); // works
msl_altitude qp1 = make_AMSL(zero); // error

I think your main point, though, is that if a user does try this, then we might get an annoying bug report. I don't think we need to worry about this. The compiler error will mention operator++(zero_t). It should be straightforward enough that one can't increment a type which is always zero.

I also think that the user will understand the compiler error but we will get a bug report anyway asking us to make it work 😉 And we will have to explain (possibly multiple times to multiple users) why it can't work and why we won't "fix" it.

The big takeaway, though, is that "zero has issues with quantity concepts"

It is not only about concepts but also about plain auto (or typename T) that people love to write in generic code.

I'm confused: why wouldn't this work? I assume q2 should have the same type and value as q1. I just did a sanity check in Au, and indeed this works:

Through the years of doing mp-units, I have learned that op+(lhs, rhs) for quantities should not allow implicit conversions because there are cases where we end up with either the ODR violation or with a lack of commutative property of addition (a + b and b + a yield different types or sometimes even only one of them compiles). This is why the op+ is implemented as follows:

template<auto R1, typename Rep1, auto R2, typename Rep2>
requires detail::InvocableQuantities<std::plus<>, quantity<R1, Rep1>, quantity<R2, Rep2>>
[[nodiscard]] constexpr Quantity auto operator+(const quantity<R1, Rep1>& lhs, const quantity<R2, Rep2>& rhs)
{
using ret = detail::common_quantity_for<std::plus<>, quantity<R1, Rep1>, quantity<R2, Rep2>>;
return make_quantity<ret::reference>(ret(lhs).numerical_value() + ret(rhs).numerical_value());
}

The above does not allow any implicit conversions. However, the compound assignment is different. It can't change the LHS type, and in the case of self-addition it means that the RHS must be of the same type. This means that we can allow implicit conversions here:

constexpr quantity& operator+=(const quantity& q)
requires requires(rep a, rep b) {
{
a += b
} -> std::same_as<rep&>;
}
{
value_ += q.numerical_value();
return *this;
}

This difference was not a big issue for now, but adding implicit conversion from zero will make them more visible (as described in my previous examples).

That said, even if it didn't, why would users be motivated to add zero to the quantity?

The zero may be just an argument of some parent function (like in the make_AMSL case above) so at the point of doing arithmetic you do not really know what is the type and value there.

In this case, we may get an extra unit conversion at runtime, between (m/s) and the common unit of v1 and v2. zero would pick the correct unit automatically

This is true, and I see this as the biggest benefit of this feature. Otherwise, we need something like this:

bool foo(QuantityOf<isq::speed> auto v1, QuantityOf<isq::speed> auto v2)
{
  const auto q = v1 - v2;
  return (q == q.zero());
}

to prevent additional conversions. This is a bit inconvenient but not that terrible IMHO.

As I wrote before, I really liked the idea of zero at the beginning and encouraged you to come forward and propose it for addition here. I even hoped a bit for a PR 😉, which I would probably accept immediately. But it actually was a good thing that I implemented it by myself because I got enough experience with this feature to understand related issues that might appear.

I see the benefits of this feature but I just don't know for now how to make it work in an easy-to-understand way. In this commit 0dd28c1 I already removed subtraction of points, and addition and subtraction of a quantity and zero. Maybe also removing the implicit construction from zero (or making it explicit if it makes any sense) will make the remaining code consistent, easy to use, and understand? But in the end, what is left is just explicit support of zero for:

  • op==
  • op<=>
  • op= (although I am not sure if that will not also cause some issues when we look closer at this use case).

Is it enough to justify the addition of a new feature?

Please let me know your thoughts.

@chiphogg
Copy link
Collaborator

I'm confused: why wouldn't this work? I assume q2 should have the same type and value as q1. I just did a sanity check in Au, and indeed this works:

Through the years of doing mp-units, I have learned that op+(lhs, rhs) for quantities should not allow implicit conversions because there are cases where we end up with either the ODR violation or with a lack of commutative property of addition (a + b and b + a yield different types or sometimes even only one of them compiles). This is why the op+ is implemented as follows:

template<auto R1, typename Rep1, auto R2, typename Rep2>
requires detail::InvocableQuantities<std::plus<>, quantity<R1, Rep1>, quantity<R2, Rep2>>
[[nodiscard]] constexpr Quantity auto operator+(const quantity<R1, Rep1>& lhs, const quantity<R2, Rep2>& rhs)
{
using ret = detail::common_quantity_for<std::plus<>, quantity<R1, Rep1>, quantity<R2, Rep2>>;
return make_quantity<ret::reference>(ret(lhs).numerical_value() + ret(rhs).numerical_value());
}

Interesting... I see that this is implemented as a nonmember function. Au takes a different approach. We haven't seen ODR violations or lack of commutativity, but maybe we have those problems and just haven't encountered them!

Our main approach to implementing quantity/quantity addition is twofold. First, we use the hidden friend idiom for same-quantity-type addition:

https://github.com/aurora-opensource/au/blob/eb615a60a0588b259b3f341279845249ce7cfd09/au/quantity.hh#L310-L312

Next, we use a nonmember function for adding two different quantity types:

https://github.com/aurora-opensource/au/blob/eb615a60a0588b259b3f341279845249ce7cfd09/au/quantity.hh#L618-L621

(There are two other overloads, but they cover the cases for corresponding quantities --- so, adding an Au quantity with one from nholthaus or the chrono library, say. I'm glossing over them for now.)

This setup has worked well so far. It preserves commutativity. It also allows us to use one-way implicitly convertible types such as ZERO. But if there are pitfalls I haven't seen, I'd like to know about it!

As I wrote before, I really liked the idea of zero at the beginning and encouraged you to come forward and propose it for addition here. I even hoped a bit for a PR 😉, which I would probably accept immediately. But it actually was a good thing that I implemented it by myself because I got enough experience with this feature to understand related issues that might appear.

I see the benefits of this feature but I just don't know for now how to make it work in an easy-to-understand way. In this commit 0dd28c1 I already removed subtraction of points, and addition and subtraction of a quantity and zero. Maybe also removing the implicit construction from zero (or making it explicit if it makes any sense) will make the remaining code consistent, easy to use, and understand? But in the end, what is left is just explicit support of zero for:

  • op==
  • op<=>
  • op= (although I am not sure if that will not also cause some issues when we look closer at this use case).

Is it enough to justify the addition of a new feature?

Please let me know your thoughts.

On reflection, I think implicit conversion is really important to get good usability. I still feel zero is worthwhile if we have it, even though it can't be passed to "generic quantity" interfaces: I guess I just value the benefits more than I worry about these costs. But if we didn't have the implicit conversions, it would be harder enough to use that I'm not confident it would be worth it.

@mpusz
Copy link
Owner Author

mpusz commented Sep 12, 2023

Interesting... I see that this is implemented as a nonmember function. Au takes a different approach. We haven't seen ODR violations or lack of commutativity, but maybe we have those problems and just haven't encountered them!

I tried making them hidden friends a few times already, but I always failed every single time because of some corner and surprising cases connected to implicit conversions of quantities.

@mpusz
Copy link
Owner Author

mpusz commented Sep 12, 2023

I thought a lot about zero in the past few days. I do not think that implicit construction from zero is a good idea for the reasons specified above. So the last one standing is the usage of zero with comparison functions.

I think we have two options here:

  1. We keep the support for zero and provide dedicated overloads for op== and op<=> like in the current implementation of this PR. This has some potential issues of people trying to do more with it like construction or assignment from zero or adding zero and complaining that it does not work and probably should. We will have to provide the arguments mentioned above of why it is not a good idea every single time.

    Usage example:

    if (q == zero) // ...
  2. We provide named non-member functions (e.g. is_eq_zero(q), is_gt_zero(q), ... similar to the interface introduced with the three-way comparison) to do the work in an efficient way.

    Usage example:

    if (is_eq_zero(q)) // ...

    This is a bit more verbose but makes it clear that the feature is meant only for comparison. Having this as a non-member function allows it to work in a generic way with other abstractions as well, which I find really nice. For example:

    template<typename T>
      requires requires { T::zero() -> std::equality_comparable_with<T>; }
    [[nodiscar]] constexpr bool is_eq_zero(T v) { return v == T::zero(); }

    the above would work with std::chrono::duration right away.

@mpusz mpusz closed this Sep 13, 2023
@mpusz mpusz deleted the mpusz/issue487 branch September 13, 2023 07:20
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.

Add support for zero
3 participants