docs(ref): document most of mp_units.core

[JohelEGP]

This is still a WIP:

• mp-units.pdf
• mp-units-html.zip

TBDs and still missing:

• TBD: The quantity specification conversion algorithms.
• TBD: The collapse common unit algorithms.
• TBD: The formatting functions for dimensions and units.
• TBD: The semantic requirements on the QuantityCharacterRepresentation concepts.
• TBD: sudo_cast.
• P1: Text output.
• P2: system_reference
• P1: math.h
• P1: random.h

[mpusz]

This is great! Thanks!

Regarding magnitudes, please note that they are meant to be implementation-defined. Only a few user-facing API things are public (construction helpers (mag, mag_ratio, and mag_power), concept, operators, magnitude<...> where ... is implementation-defined).

[mpusz]

This is huge!!! Lots of great work! Thank you!!!

[mpusz]

How can we specify that some public identifiers should not be exported from the std module?

[JohelEGP]

Reading https://wg21.link/std.modules, it seems like there's no way.

[mpusz]

I will ask LWG members

[JeffGarland]

Dropping in mid-stream here -- is there a problem with marking them as exposition only? That basically says -- we're going to act like this thing exists in the standard, but we might implement it differently or call it something different.

[mpusz]

exposition-only is not a solution, as I wrote in the LWG reflector.

[mpusz]

There are some users that derive from quantity already. We support this in mp-units with derived_from. Should we remove this support?

[JohelEGP]

Should we remove this support?

I've always been against it.
Do you have the related issue/PR numbers?
I hope to convince you to do that with my TBD review here.

[mpusz]

#271
#286

[mpusz]

This is controversial for me as well. In C++ we typically discourage people from inheriting from std types. It would be great if we could find a better solution here.

[JohelEGP]

The most glaring issue is that for a parameter constrained with
std::derived_from<quantity> or std::derived_from<quantity_point>,
the implementation doesn't convert to the quantity base subobject.

For example, in operator+:

  template<std::derived_from<quantity> Q, auto R2, typename Rep2>
    requires detail::CommonlyInvocableQuantities<std::plus<>, quantity, quantity<R2, Rep2>>
  [[nodiscard]] friend constexpr Quantity auto operator+(const Q& lhs, const quantity<R2, Rep2>& rhs)
  {
    using ret = detail::common_quantity_for<std::plus<>, quantity, quantity<R2, Rep2>>;
    const ret ret_lhs(lhs);

We need this conversion from Q, type of lhs, to ret,
to be the same as ret(static_cast<const quantity&>(lhs)).
The implementation doesn't do that (like as-variant),
so we need wording to require those parameter types
to not hide members
and prevent conversions like the above to have a different meaning.
Perhaps by requiring Q to behave as-if the base template in the specified wording
(e.g., by borrowing https://eel.is/c++draft/namespace.std#2.2).

[mpusz]

I do not oppose removing the support for deriving from a quantity.

[mpusz]

Could you please not squash or rebase the branch?

It is impossible to track what changes between commits this way. I need to start reviewing all 2500 lines from scratch as I do not know what has changed.

Maybe provide something for merge and open another PR with additional changes?

[mpusz]

I try to review in LatTeX as it is easier to provide comments that way, and I also need to learn the syntax.

[mpusz]

Even if it looks right, it still might be broken. This is why I asked to have all the changed code in the repo.

[mpusz]

Huge work @JohelEGP! Thank you so much for doing this!

[mpusz]

Thank you!

[JohelEGP]

As an implementation, how would mp-units define this behavior?
Here's my take on it.

• There are terms for powers of each prime number that represents the magnitude.
  [ Note:
  It doesn't matter that above reads "[t]here is a single term for each base-type".
  mag<INTMAX_MAX> * mag<INTMAX_MAX> is implementation-defined.
  But mag<INTMAX_MAX> having more than one term with base-type = std::intmax_t is unobservable.
  -- end note ].
• mag_power<2, INTMAX_MAX> * mag_power<2, 1> naturally fails
  (although mag_power<2, INTMAX_MAX> * mag_power<3, INTMAX_MAX> doesn't).
• Big bases just run into the limits of constexpr evaluation
  (although there's work going around to push this limit).

[mpusz]

Sure, I am not sure if that worked in C++20 already, though. So, as long as doing it for API reference is fine, it might break the mp-units code.

[JohelEGP]

Is it OK to use (Den != 0) instead of detail::non_zero<Den>?

[mpusz]

Sure! Someone added those math concepts but I also consider them often more an issue than help. If you are OK with that, I can remove them from the code and replace with proper expressions.

[JohelEGP]

This kind of ratio in template parameters only require that Den != 0.
However, ratio{Num, Den} would fail when Abs or Den is INTMAX_MAX due to abs.
Is that a problem in any of these interfaces?

[mpusz]

I don't know. I did not think about it so far. We may need dedicated Issues to track things like that.

[mpusz]

Actually, it is even better (or worse). It turns out that all_are_kinds are not needed anymore. It was needed at the beginning when I allowed any quantity spec to be used for a unit. Now we only allow kinds, so they are always kinds for an AssociatedUnit.

[JohelEGP]

Are these std::remove_reference_t<Q> for Clang diagnostics?
Q is defaulted to std::remove_cvref_t<FwdQ>.

[JohelEGP]

Hmm...
This constructs a quantity point with a different point origin
and delegates to the converting constructor,
which just extracts the quantity type member.
There's no need to go out of our way to construct the quantity point.
Let's just initialize the quantity type member directly with q.

[JohelEGP]

The constraint already includes convertibility to quantity_type.
Its negation in the explicit-specifier will never be exercised.

Do you think the same applies to the equivalent constructor of quantity?
Construction is done with sudo-cast, so it isn't as evident.
But I suppose that implies convertibility.

Also, the same applies to the conversion operators
from quantity to some QuantityLike and
from quantity_point to some QuantityPointLike.

Their deduction guides already do it right.

[JohelEGP]

In code, this is:

  template<detail::Mutable<quantity_point> QP, auto R2, typename Rep2>
    requires detail::QuantityConvertibleTo<quantity<R2, Rep2>, quantity_type> &&
             requires(quantity_type q) { quantity_from_origin_is_an_implementation_detail_ += q; }
  friend constexpr decltype(auto) operator+=(QP&& qp, const quantity<R2, Rep2>& q)
  {
    qp.quantity_from_origin_is_an_implementation_detail_ += q;

requires(quantity_type q) { quantity_from_origin_is_an_implementation_detail_ += q; }
should have been
requires(quantity<R2, Rep2> q) { quantity_from_origin_is_an_implementation_detail_ += q; }
I suggest just using a trailing requires-clause that uses the actual expression.
That would also make the detail::QuantityConvertibleTo redundant.
Same for operator-=.

[JohelEGP]

Should QP.quantity_from_zero() actually be QP.quantity_from(QP.absolute_point_origin)?

[mpusz]

Is there a good reason for this change? Should I also update the sources? I used :: to ensure that we will not have mp_units::mp_units by accident 😉

[JohelEGP]

Is there a good reason for this change? Should I also update the sources?

I thought that's how the C++ Standard library did it.
Now I searched for ::std::, and found that this is also used in 2 places:

• d.operator ::std::basic_string_view in basic_string_view::basic_string_view.
• In [locale.facet], a requirement on an id member.

Besides CPO(-like)s, move, forward, compatibility subclauses, and examples,
just std:: is used for

• [format]'s types' use of std::locale (since they have a locale member).
• prm's using X = std::X.
• container(-like)'s using reverse_iterator = std::reverse_iterator,
• span's using const_iterator = std::const_iterator,
• span's use of std::data (since it has a data member),
• view's use of std::min (since std::ranges has a min),
• view's use of std::get (since std::ranges has a get),
• [alg.reverse]'s use of std::iter_swap (along with ranges::iter_swap),
• linalg types' use of std::extents (since they have an extents member),
• I/O types' use of std::span (since they have a span member),
• path's std::Xstring Xstring() members, and
• [future.state]'s use of std::async (for no apparent reason),
• [future.async]'s use of std::bad_alloc (for no apparent reason),

std::chrono::zoned_traits, which has a locate_zone member, uses
std::chrono::locate_zone instead of
::std::chrono::locate_zone or
chrono::locate_zone.

So I might prepend :: to all occurrences of mp_units::.
I don't know if it'd be worth it, considering that these names might not live in ::std.

I used :: to ensure that we will not have mp_units::mp_units by accident 😉

How is that possible?

[mpusz]

The probability is low, but there is always a chance to have incorrect curly braces in headers or a user that will do some strange thing. Anyway, I am fine with mp_units:: as well. I just wanted to check if there is a bigger reason that I am not aware of.

[JohelEGP]

That's just bad practice.
Everyone should be following [using.headers]p3 unless otherwise documented for the header.

I'm making this change right now.
You don't follow it in the specializations for std::common_type of quantity.

[JohelEGP]

There are some things I still want to do, but you can merge this for now.
I also have pending fixing the HTML url bug in the index.html.

[mpusz]

Thanks a lot!!!