Hidden Friends issue about mp-units HOT 24 CLOSED

Whatever way you look at it, that looks broken in the compiler or if its correct, the language

https://en.cppreference.com/w/cpp/language/friend

"A name first declared in a friend declaration within class or class template X becomes a member of the innermost enclosing namespace of X!

IOW, in that context, the friend function should behave exactly like the non-friend function

from mp-units.

The friend version arbitraily picks the lhs operand and decides to convert it, while the non-friend politely declines to make an arbitrary decision as shown below. This is why historically I have always avoided binary operators defined within a class. They dont ever seem to give equal weight to both operands. proved again here! Further you can see the semantic issue in the non friend function, which is not obvious due to the mental gymnastics in the friend version. Generically one always needs to provide a more balanced solution for the result of a binary operation than just choosing the type of the lhs operand, or indeed converting it to the type of the right operand!

friend version
https://godbolt.org/z/2wbJD-

non friend
https://godbolt.org/z/ywkupG

from mp-units.

Hello,

I'd like to know why you expect the assertion in the friend-version to fail?

After all, X<T,U> is implicitly constructible from X<T2,U2>. Even in the non-friend-version, code like this should compile:

static auto add(X<int, int> x1, X<int, int> x2) {
    return x1 + x2;
}
int main() {
    add(X<int, int>{1}, X<float, float>{1});
}

The friend-version actually seems to be more consistent.

from mp-units.

Re-reading my comment... let me rephrase this more politely. I'm not questioning your expertise here!

As I understand it, X denotes a quantity, T the value type (int, double, etc...), and U denotes the unit (metres, centimetres, seconds, etc...)

My question now is, why is there an implicit conversion between (probably) incompatible units?

The implicit conversion should be properly restricted and only be allowed for compatible units (i.e. same dimensions/kinds).
And in order to make operator+ symmetric, the return type should be the "common" type Q = X<decltype(lhs.x_ + rhs.x_), U> and should be disabled if the rhs is not implicitly convertible to Q.

With these changes, the code works as I would expect it to work. Here is a slightly rewritten version of your friend-version: https://godbolt.org/z/47BBEV

Note that the code is still missing implicit conversions e.g. from metres to centimetres, but it is not hard to add these conversions. (I have used this style - implementing arithmetic operators etc. as friend functions - in my own units library and up until now, it works quite well.)

from mp-units.

@abolz I think the point is that friend is unnecessarily complicated and confusing. (A solution was provided)For Hidden Friend in example The solution is given:

1. In friend binary op make sure both args have equal weight ( either both a X or both a X<Tn,Un>)
2. In friend binary op Provide constraints so that only one overload can be created by making one arg an X without explicitly naming it ( ODR rule)

Question was: Should we provide constrained hidden friend version of the operator or non-friend one with all its overload resolution issues?

There is an assumption there that non-friend has overload resolution issues, but does it?

Is overload resolution a problem in practise or is it a theoretical issue? Or is it an issue of old days?
finding incorrect functions via ADL was maybe long ago a problem of unconstrained templates, but first we got SFINAE constrained templates and now concept constrained templates as well.

In favour of free function is that it is simple to understand. For me that has a lot of weight. I prefer to use constraints to solve overload resolution

Drilling down in the Hidden Friend, the real "killer application" reason for use is that it may reduce compile times. (So test how much difference it makes) If proved that too carries a lot of weight. And for an implementation that may be a good reason to prefer it.

Against Hidden Friend. Do we need more C++ "smart ass" stuff?

Also by definition a friend is a friend of a class template and is therefore not as generic as a free function. For example, where two classes model some concept, no reason why one function cannot serve:

https://godbolt.org/z/FnLA7Z

Furthermore the class interface there is much simpler and cleaner without the friend/operator function junk, therefore much easier to deal with. Clean simple to understand code is maintainable and good code ( One of the rationale behind Quantities)

from mp-units.

The problem with friend unconstrained version is more complicated than in the simplified example above. With such an implementation:

template<typename U2, typename Rep2>
[[nodiscard]] friend constexpr Quantity AUTO operator+(const quantity& lhs, const quantity<D, U2, Rep2>& rhs)
  requires detail::basic_arithmetic<Rep, Rep2>
{
  using common_rep = decltype(lhs.count() + rhs.count());
  using ret = common_quantity<quantity, quantity<D, U2, Rep2>, common_rep>;
  return ret(ret(lhs).count() + ret(rhs).count());
}

I noticed the following asymmetry:

static_assert(cgs::length<cgs::centimetre, int>(100) + si::length<si::metre>(1) == si::length<si::metre>(2));
static_assert(si::length<si::metre>(1) + cgs::length<cgs::centimetre, int>(100) == si::length<si::metre>(2));

From the above only the second one fails with:

error: no match for ‘operator+’ (operand types are ‘units::si::length<units::si::metre>’ {aka ‘units::quantity<units::si::dim_length, units::si::metre, double>’} and ‘units::cgs::length<units::si::centimetre, int>’ {aka ‘units::quantity<units::cgs::dim_length, units::si::centimetre, int>’})

With a constrained friend on non-friend version both cases fail.

There is an assumption there that non-friend has overload resolution issues, but does it?

Yes, it does. Compiler implementers claim that the overload resolution on common operators is the most expensive operation that the compiler does and takes the most of the compile-times. This is why Modern C++ design conventions strongly suggest using Hidden Friends and it might be even hard to standardize the library without it (or without a good excuse).

from mp-units.

That's probably because common_quantity is not symmetric,
and si::length<si::metre> is implicitly convertible to cgs::length<cgs::centimetre, int> (but not vice versa)

https://godbolt.org/z/Pmst5C

from mp-units.

Yes, it does. Compiler implementers claim that the overload resolution on common operators is the most expensive operation that the compiler does and takes the most of the compile-times. This is why Modern C++ design conventions strongly suggest using Hidden Friends and it might be even hard to standardize the library without it (or without a good excuse).

So Basically the real reason for hidden friends is an optimisation for faster compiler lookup, at expense ( personal opinion ) of clarity.

Bearing in mind the classic phrase "early optimisation is the root of all evil" , the namespace level functions can still be useful for documentation. They separate the class internal machinery ( data, construction, destruction, conversion) from its operations, which (personal opinion) helps legibility. Maybe provide a note in the reference documentation that for faster lookup, the implementation may optimize functions x,y,z as hidden friends of the class template (so long as the semantic of the function is not affected apart from differences in lookup). You win in helping users understand the semantics of the class and its operations and by allowing library authors freedom to implement efficiently. For some uses, e.g to teach and understand the class template, the namespace level function may be more appropriate (and also incidentally in teaching the optimisation iself and issues around it).

from mp-units.

That's probably because common_quantity is not symmetric,
and si::length<si::metre> is implicitly convertible to cgs::length<cgs::centimetre, int> (but not vice versa)

https://godbolt.org/z/Pmst5C

The first commented out static_assert fails by design I think due to narrowing conversion
https://github.com/mpusz/units/blob/master/src/include/units/quantity.h#L42

 static_assert(units::detail::safe_convertible<double,int>,""); // fail

I have to confess that the common_quantity and its rationale eludes me so far however ( maybe I need to read the docs there)

from mp-units.

Ah! I thought the intent was to disallow conversions between different systems (cgs and si).

Then - since both Q1 = si::length<si::metre> and Q2 = cgs::length<cgs::centimetre, int> are implicitly convertible to common_quantity<Q1, Q2> (and common_quantity<Q2, Q1>) - I think that both assertions should pass.

from mp-units.

Ah! I thought the intent was to disallow conversions between different systems (cgs and si).

Then - since both Q1 = si::length<si::metre> and Q2 = cgs::length<cgs::centimetre, int> are implicitly convertible to common_quantity<Q1, Q2> (and common_quantity<Q2, Q1>) - I think that both assertions should pass.

(Simplifying)
https://godbolt.org/z/BkTqXp
should answer the question , but you will have to ask @mpusz why it does that!

Personally I am confused why a dim_length is differentiated between the two systems or indeed why it is coupled to the unit at all.
https://github.com/mpusz/units/blob/master/src/include/units/base_dimension.h#L50
#44

from mp-units.

That's probably because common_quantity is not symmetric,

This asymmetry was not intentional and probably should be fixed.

from mp-units.

Bearing in mind the classic phrase "early optimisation is the root of all evil" , the namespace level functions can still be useful for documentation. They separate the class internal machinery ( data, construction, destruction, conversion) from its operations, which (personal opinion) helps legibility.

Please note that the hidden friend (or any friend function) is always non-member (namespace level) function.

Maybe provide a note in the reference documentation that for faster lookup, the implementation may optimize functions x,y,z as hidden friends of the class template (so long as the semantic of the function is not affected apart from differences in lookup).

Hidden friends are not just optimization and cannot be specified as suggested above. There are already concrete rules on how to specify them in the wording section of the proposal (https://wg21.link/p1601).

from mp-units.

I have to confess that the common_quantity and its rationale eludes me so far however ( maybe I need to read the docs there)

The rationale for common_quantity came from https://en.cppreference.com/w/cpp/chrono/duration/common_type. When I started I wanted to stay as close to std::chrono::duration design as possible. Hower, now we are so far from it any way that we can refactor this design. Any ideas or Pull Requests are welcomed ;-)

from mp-units.

Personally I am confused why a dim_length is differentiated between the two systems or indeed why it is coupled to the unit at all.

Well, there is a popular demand to support multiple systems of units and provide cooperation between them. Here is a list of the most important requirements:

• A quantity should be somehow connected to its system (either via an additional template parameter, or a special version of current template parameters).
• Printing of a quantity unit symbol in terms of its base units should use correct units for the current system of units (https://github.com/mpusz/units/blob/master/test/unit_test/runtime/fmt_test.cpp#L383-L424).
• The library should provide support to define a quantity that has the same dimension (i.e. length) but different units (i.e. km and Mpc) in numerator and denominator (i.e. Hubble parameter).
• We should be able to compare quantities of the same dimension from different systems.

I think that the easiest way to support the above is to make a base_dimension aware of it unit so we have si::dim_length : base_dimension<"L", metre> and cgs::dim_length : base_dimension<"L", centimetre>. However, if you have a better idea of how to make it work without additional specification lines (i.e. type trait specializations) than please let me know.

Also, we can discuss on how binary operators (+, -, *, /) on units from different systems should work. Right now I assumed that for:

• + and - units should be explicitly cast to the same system before the operation (so we know the resulting system of units).
• * and / units may be from different systems but in such a case we will end up with an unknown dimension. Afterward, we should be allowed to explicitly cast such a dimension to a known dimension in a specific system. (Note that the second step is still not implemented in the library)

As always I am really open to any feedback here as there is not too much experience in the industry of supporting multiple systems and the author of the only library that explicitly supports it (Boost.Units) claims it was one of the biggest issues in the library design and got rid of it in the refreshed version (it was never finalized).

from mp-units.

Well, there is a popular demand to support multiple systems of units and provide cooperation between them. Here is a list of the most important requirements:

* A quantity should be somehow connected to its system (either via an additional template parameter, or a special version of current template parameters).

* Printing of a quantity unit symbol in terms of its base units should use correct units for the current system of units (https://github.com/mpusz/units/blob/master/test/unit_test/runtime/fmt_test.cpp#L383-L424).

* The library should provide support to define a quantity that has the same dimension (i.e. length) but different units (i.e. `km` and `Mpc`) in numerator and denominator (i.e. Hubble parameter).

I think there is another layer to the onion, which is what I am caling below a quantity_universe. Here is what I wrote yesterday to try to explain that. ( mainly for my own benefit)

Physical-quantities are members of a quantity_universe. Examples of quantity_universe are e.g Newtonian, Einsteinian.

Base_quantities are the primitives of a quantity_universe. Any physical quantity in the universe can be described in terms of a combination of the universes base quantities
base-quantities are combined in a limited algebra known as dimensional analysis to create other physical quantities.
An example base_quantity in Newtonian universe is length which has rank 1. In dimensional analysis algebra, for example, multiplication of a length by a length gives a length of rank 2 which can describe an area.
Quantities that can be combined using dimensional anlysis are referred to as abstract_quantities, since they have no notion of a numerical value.
Abstract_quantities exist in a quantity_universe, but outside a quantity_system.

Quantity system
A quantity system provides a set of rules for ascribing numerical values and arithmetic operations on physical_quantities that exist within a quantity_universe.
Attaching numerical values to physical quantities allows meaningful arithmetic calculations involving their absolute and relative magnitude.
Physical quantities that have meaningful numerical values are referred to as concrete_quantities.

Examples systems si, cgs ,mks

SI system acts in newtonian_universe. SI system uses physical measurements to calibrate a set of base_units, which allow a physical quantity to be consistently
and accurately described by a numerical value, representing a magnitude of the quantity relative to the physical phenomenon it was calibrated from.

si conversion factor
Numerical values of concrete_quantities in Si system can be modified by means of conversion factor. The conversion factor has 2 parts , an exponent and a multiplier.
The exponent parts means that the numeric value of the quantity is multiplied by positive and negative rational exponents of base 10
The multiplier part allows conversions from physical quantities whose numeric value represents a value in another system in the newtonian universe.

from mp-units.

@kwikius Really nice overview! In general, I agree with you. There are many approaches to look into this design space. I personally prefer to stick with official ISO 80000 terms and definitions that you can find in https://wg21.link/p1935. There:

A base quantity is a quantity in a conventionally chosen subset of a given system of quantities, where no quantity in the subset can be expressed in terms of the other quantities within that subset.

A base unit is a measurement unit that is adopted by convention for a base quantity. In each coherent system of units, there is only one base unit for each base quantity.

The above states clearly that a base quantity is tightly assigned to a system of quantities and each of them has its base unit. This is what I tried to reflect in my design.

I am not sure if you are familiar with the following header file: https://github.com/mpusz/units/blob/master/src/include/units/physical/dimensions.h. With it, I tried to represent what you call a base quantity. For example dim_length is a base quantity (everything that has the same identifier is the same physical base quantity). The fact that it is a template that takes a unit is just an implementation detail that allows us to implement multiple systems support and base units that are adopted by conventions for base quantities in those systems.

from mp-units.

As a side note, it is nice to know the correct terms and definitions. However, I also think that we need to make a distinction here between theory, implementation, and teachability. We need a library that is easy to learn and use. I am trying the best I can to represent theory in the design. Of course, no code is perfect and my library is not perfect for sure too. If you have a good idea of how to redesign it or rename some terms please submit a Pull Request and we can discuss it there.

from mp-units.

I prefixed my answer with 'mainly for my own benefit'

It is hard to accept the answer to the question of how to support systems outside the SI, is to look in the SI documentation ;)

My interest is the SI. My recommendation is "Use the SI' My wish is a standard units library that supports the SI. 'multiple systems' ? couldnt care less personally...

but you earlier brought up the expressed desire to support multiple systems of units, for which I provide an idea of this next layer of the onion, so now I suggest that the advocates for these "multiple systems" need to provide some more detailed examples of what these other unit systems are that they need so badly, where are the c++ examples of how they are using these systems, what is inadequate for them about the conversions provided in the SI and also please show us their terminology and documentation. The answer usually is physicists, there arent many of them but they are extremely well fed and noisy ;) Meanwhile I prefer to concentrate on the needs of the starving millions of engineers!
As a start an example of a popular C++ application desperately requiring hubble constant would be great ;) I googled for such an application, but no luck so far...

I think mpusz/ units lacks real code examples, so I will try to convert some of the examples from my library and see how I get on:
https://github.com/kwikius/quan-trunk/tree/master/quan_matters/examples

I

from mp-units.

It is hard to accept the answer to the question of how to support systems outside the SI, is to look in the SI documentation ;)

Totally agree 😉

A common request is CGS but more extreme systems are also requested. You can find those in chapter 4.4 of Walter's Brown paper (note that it was written more than 20 years ago). Some other folks explicitly named Planck Units. And yes, they are mainly physicists 😉

There is also a dedicated paper Towards a standard unit systems library from Vincent Reverdy with requirements for the units library.

I think mpusz/ units lacks real code examples, so I will try to convert some of the examples from my library and see how I get on:

Great, thank you!

from mp-units.

A common request is CGS but more extreme systems are also requested. You can find those in chapter 4.4 of Walter's Brown paper (note that it was written more than 20 years ago). Some other folks explicitly named Planck Units.

There is also a dedicated paper Towards a standard unit systems library from Vincent Reverdy with requirements for the units library.

I am really excited to see the reference implementation to go with this. (Unfortunately I missed the link in the paper.. )
Is this the same Vincent Reverdy?
https://github.com/vreverdy/
https://www.nsf.gov/awardsearch/showAward?AWD_ID=1642411&HistoricalAwards=false

What has he said about mpusz/units?

from mp-units.

Yes, Vincent is aware of this library and we meet regularly on the ISO C++ Committee meetings. The next one will be in a month from now in Prague.

from mp-units.

This is one way to solve the problem https://godbolt.org/z/zGPbBz

from mp-units.

I ran into this: https://godbolt.org/z/KGr8cq.
operator%(MyInt<T>, MyInt<U>) is not viable, so operator%(MyInt<U>, MyInt<U>) is considered and MyInt<T> converted to MyInt<U>, and the operation I wanted to prevent succeeds.
Using const same_as<MyInt<T>> auto& as the first parameter, as you essentially do in the solution of the OP, works with a not so ideal error message: https://godbolt.org/z/qh8Whc.

With this, we achieved what we wanted but with a cost of a more complicated signature.

Is that the case if you use const std::same_as<quantity> auto& as the first parameter?

    template<typename TT, typename U1, typename U2>
        requires std::same_as<TT, T> && std::same_as<U1, U>
    friend constexpr X operator+(const X<TT, U1>& lhs, const X<TT, U2>& rhs) {

becomes (https://godbolt.org/z/fGY8GE)

    template<typename U2>
    friend constexpr X operator+(const std::same_as<X> auto& lhs, const X<T, U2>& rhs) {

from mp-units.