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

Poll: UDLs vs constants #48

Closed
mpusz opened this issue Dec 29, 2019 · 19 comments · Fixed by #184
Closed

Poll: UDLs vs constants #48

mpusz opened this issue Dec 29, 2019 · 19 comments · Fixed by #184
Labels
help wanted Extra attention is needed Polls
Milestone

Comments

@mpusz
Copy link
Owner

mpusz commented Dec 29, 2019

The more I think about it the more I think that UDLs may not be the right tool for the job. Yeah, we used it in std::chrono and it seems nice to write auto v = 120km / 2h;. However, they could be replaced with constants. For example:

namespace si {
inline namespace unit_constants { // is it a good name?

inline constexpr length<metre> m(1);

}
}

Here are a few issues that I see with UDLs:

  1. UDLs are only for compile-time known values. Currently with UDLs:

    using namespace units::si;
    auto v1 = 120km / 2h;
    auto v2 = length<kilometre>(length) / time<hour>(duration);

    with constants those 2 cases would look like:

    auto v1 = 120 * km / 2 / h;
    auto v2 = length * km / duration / h;

    Constants treat both cases in a unified way. It is also worth to notice that we work mostly with runtime variables and compile-time known values appear only in physical constants and unit tests.

  2. UDLs for some units may be impossible to achieve in C++. I already found issues with F (farad), J (joule), W (watt), K (kelvin), d (day), l or L (litre), erg, ergps. It is probably still not the complete list here. All of those problems originated from the fact that those numeric symbols are already used in literals (sometimes compiler extensions but still). I am afraid that at for some of those cases we will not be able to fix it or workaround it in the C++ specification. None of those issues affect constants.

  3. UDLs cannot be disambiguated with the namespace name:

    using namespace si;
    auto d = 1cm;   // quantity<si::dim_length, si::centimetre>
    using namespace cgs;
    auto d = 1cm;  // quantity<cgs::dim_length, si::centimetre>
    using namespace si;
    using namespace cgs;
    auto d = 1cm;   // FAILS TO COMPILE

    With constants it is simple:

    using namespace si;
    using namespace cgs;
    auto d1 = 1 * si::cm;   // quantity<si::dim_length, si::centimetre>
    auto d2 = 1 * cgs::cm;  // quantity<cgs::dim_length, si::centimetre>

So maybe we should prefer constants over UDLs? If so should they be put to an inner inlined namespace (like literals)?

Let's vote:
(please vote only once by clicking on the correct bar above)


@polls polls bot added the Polls label Dec 29, 2019
@mpusz mpusz added the help wanted Extra attention is needed label Dec 29, 2019
@mpusz
Copy link
Owner Author

mpusz commented Dec 29, 2019

Related issue #31.

@kwikius
Copy link
Contributor

kwikius commented Dec 31, 2019

UDLs are simple shorter and neater so get my vote . Ultimately if they have to have slightly longer names then that is not a problem as far as I am concerned.
UDLs is a neat idea that unfortunately causes very short names to appear in global scope due to unqualified lookup only allowed.
Reserving literals operator names without underscores to the standard namespace didnt solve anything anyway since they are so useful that they are already colliding in std namespace :)

A nice solution would be to extend UDL rules to allow qualified lookup. Here is one possible syntax. I think scope-resolution-operator followed by a number is syntactically available

  auto x1 = si::1mm; 

  auto x2 = si :: 1 mm;  // with the scope resolution operator prefix  
                         // pretty spaces should be parsable  

That would bring them to a similar functionality to inline constants.

EDIT: Actually lthe above syntax could mean that if si::mm is a type with an explicit constructor taking an int, construct a constexpr object of that type with that argument. No literal operator required!

Meanwhile the obvious pragmatic workaround is to attach a postfix namespace to avoid collisions which are otherwise inevitable as use grows

auto x2 = 1l_si;

@mpusz
Copy link
Owner Author

mpusz commented Jan 1, 2020

Qualified lookup for UDLs was discussed already in the ISO C++ Committee but if I recall correctly it did not fly (at least so far). But even if it did it would solve only point 3 from the above list. Still, we cannot easily create some UDLs and long names do not help in some cases (i.e. erg or ergps). Also as I noted in point 2 UDLs help only when we have compile-time known constant. Otherwise, we have to decay to the long form of variable initialization.

But yes, I also always hated the "multiply" syntax to form quantities but it seems it actually may have some benefits over UDLs.

@kwikius
Copy link
Contributor

kwikius commented Jan 1, 2020

There is another option. Just use a temporary as you show above,
I show again my typedef syntax which works very well and could be used by mpusz units . It contains all the information required in the shortest form for the commonest quantities and coherent units

   length::mm{1} 
      // or ...
   length::mm<>(1) // to expose the value_type default to double

Anyway, certainly worth looking at where and how often the use of such constants occurs in real world. How common is it and is it necessary to define constants at global scope versus just temporaries with explicit initialisers as I used to do before UDLs? (N.B. Discussing quality of my code and how it could be improved is not the point here ;). It is just shown as an example of day to day use. The code could certainly benefit by review, upgrading to UDLs etc,etc)

To answer point1 I would take the UDL where it works and use the temporary where not
Initialisation is a very common programming chore in application development.
Looking through my code I tend to try to use a short local typedef and then a rvalue with explicit number init for a constant ( remembering I used these types for many years before UDLs were available)
https://github.com/kwikius/aerofoil/blob/master/aerofoilDoc.cpp#L11
// mm is typedef in class definition, could be replaced by mpusz quantity
https://github.com/kwikius/aerofoil/blob/master/aerofoilDoc.hpp#L21

I think using 1 * si::mm here would start to get on my nerves!

in for loop you would hope for a terse syntax.
https://github.com/kwikius/quan-trunk/blob/master/quan_matters/examples/capacitor_time_curve.cpp#L55

for ( auto t = 0ms ; t <= timeout; ++t  ){     //nice when it works !
for ( auto t = 0 * si::ms ; t <= timeout; ++t  ){ //  maybe !
for ( auto t = si::0ms ; t <= timeout; ++t  ){  // maybe !

In class constructors generally I found explicit init with an explicit value easiest), so UDL irrelevant
https://github.com/kwikius/mavlink_to_frsky/blob/master/stm32f4/aircraft.hpp#L83

Another answer : Provide both options for now ... :)

@kwikius
Copy link
Contributor

kwikius commented Jan 1, 2020

Actually I am starting to change my mind. Maybe the multiplier syntax is not so bad :)

@mpusz
Copy link
Owner Author

mpusz commented Jan 1, 2020

Hehe, I see you are going the same path as I did. My initial opinion was also "yack, it is an awful and old way to do it" ;-)

@mpusz
Copy link
Owner Author

mpusz commented Jan 1, 2020

With constants, we could also consider more extensions like:

inline constexpr auto km = k * m;
inline constexpr auto kmph = km/h;

But as we discussed in another thread that will work against the downcasting facility as the user is never providing a user-friendly name for a target type.

@i-ky
Copy link
Contributor

i-ky commented Jan 1, 2020

with constants those 2 cases would look like:

auto v1 = 120 * km / 2 * h;
auto v2 = length * km / duration * h;

Won't these expressions produce km×h instead of km/h?

@mpusz
Copy link
Owner Author

mpusz commented Jan 2, 2020

@i-ky You are right, I do not have experience with this syntax and did not notice that. But it seems it is a good point against the "multiply" syntax. I will fix the code sample in polls above, thanks!

@kwikius
Copy link
Contributor

kwikius commented Jan 2, 2020

Another point against UDLs. Arbitrary expressions are not allowed

   auto x = (1./4)sq_m; // error

But a major point against constants is that they clog up the global namespace with many tiny names , h, s, etc. and that could be a real headache in real scale code .

    auto constexpr v1 =  10 * m/s;

    std::cout << v1 << '\n';

    double constexpr s = 2.0;

    auto v2 =  20 * m/s;
     
    std::cout << v2 << '\n';

https://godbolt.org/z/_LcyNt

That doesnt happen with UDLs'.

@kwikius
Copy link
Contributor

kwikius commented Jan 2, 2020

Personally I dont think item 1 regarding initialisation of quantitites by runtime numeric values on the list above is a problem in practise. Generally a variable quantity is initialised with a constant , but once initialised, it is the quantity that is being used as the runtime variable. Where the UDL syntax lacks recommend to use a temporary.

In light of my point about name hiding in the post above, I think that UDLs are the right way to go. Since it is not possible to use qualified lookup in current c++, the only other option is to add a namespace to the name itself as in the good old C days.

So for example reserve the Q_ prefix for use by std::quantity literals

   auto v1 = 10.0Q_F;
   auto v2 = 10.0Q_J;
   auto v3 = 10.0Q_W;
   auto v4 = 10.0Q_K;
   auto v5 = 10.0Q_d;
   auto v6 = 10.0Q_l;
   auto v7 = 10.0Q_L;
   auto v8 = 10.0Q_erg;
   auto v9 = 10.0Q_ergps;

EDIT: in line with everything is std being lowercase, and also becase it is easier to read against the preceding digit. Anyway the style has a lot of appeal to me as I test it. The uniform prefix lets you know immediately that the type is a quantity, rather than for example a chrono duration or another name for a long

   auto v1 = 10.0q_F;
   auto v2 = 10.0q_J;
   auto v3 = 10.0q_W;
   auto v4 = 10.0q_K;
   auto v5 = 10.0q_d;
   auto v6 = 10.0q_l;
   auto v7 = 10.0q_L;
   auto v8 = 10.0q_erg;
   auto v9 = 10.0q_ergps;

That solves all items. I think it is correct that the si units take precedence over the other less complete unit systems. SI is the dominant system for very good reason. For disambiguating non si units, then just extend the namespace. The extra ugliness is justified:

auto v10 = 10.0Q_imp_in;

@oschonrock
Copy link
Contributor

I think we need feedback from a wider audience on this.

It might make sense to retain both options.

@mpusz
Copy link
Owner Author

mpusz commented Feb 16, 2020

I think q_ prefix has a lot of sense. I will make this change soon. It will address #31.

Also, during the last ISO discussion it was raised that there should be a dedicated operator:

quantity<Dim, U, Rep> operator*(Rep v, magic_type<Dim, U>);

It will only allow the syntax 3 * s where:

inline cosntexpr magic_type<dim_time, second> s;
  • It will solve the issue with op/ (as it will be an illegal operation)
  • We can deduce Rep for quantity from the LHS type of the operator
  • It is easy to disambiguate different systems

I think that both options seem interesting and thus we will probably provide both for now and get some more field experience with both of them. Do you have any suggestions on how to name a magic_type?

@mpusz mpusz pinned this issue Feb 25, 2020
@mpusz mpusz changed the title UDLs vs constants Poll: UDLs vs constants Feb 25, 2020
@JohelEGP
Copy link
Collaborator

JohelEGP commented Sep 8, 2020

Both options are the way to go. Sometimes you want to encode a constant, others, do apples / h, which I prefer very much over apples / hours{1}.

I like magic_type, so I don't have to write apples / h<int> (like with std::numbers). But you mention that operator/ would be ill-formed. I think everything should be allowed everywhere.

I make a point for having a natural syntax when writing formulas with a units library at nholthaus/units#196. In C++20, you can write 1970y/January/1. A units library shouldn't lose to that.

Looking for formulas at https://en.wikipedia.org/wiki/Formula, we see:

units::pow<3>(cm) should work and be the intuitively expected magic_type specialization.
Some formula look like A = x * u1 * u2 / u3, where ux are units. Maybe I have a quantity a representing x * u2 / u3, so I only need to do A(a * u1) in C++.

I think magic_type should just be a quantity with a library-defined representation type that does the right thing when it is involved in an operation with any other representation type, so that everything else remains unchanged.

@JohelEGP
Copy link
Collaborator

JohelEGP commented Sep 9, 2020

On a second thought, there'd be cm3, so units::pow<3>(cm) is too round-about, but feasible still when the library doesn't have such constant. Also, point 2 in the OP still applies to the names of these constants.

Another advantage of constants relates to the N*M problem. The library only offers q_m_per_s and q_km_per_h, but maybe I want q_km_per_s. With constants, this isn't a problem.

@kwikius
Copy link
Contributor

kwikius commented Sep 15, 2020

On a second thought, there'd be cm3, so units::pow<3>(cm) is too round-about, but feasible still when the library doesn't have such constant. Also, point 2 in the OP still applies to the names of these constants.

Another advantage of constants relates to the N*M problem. The library only offers q_m_per_s and q_km_per_h, but maybe I want q_km_per_s. With constants, this isn't a problem.

A major practical problem with global constants is they can be hidden by local constants of the same name :

// some short name global constant

double  constexpr s = 0.5;

//----------------------------------
#include <iostream>

int main()
{

   constexpr auto a=1.,b=2.,c=3.,s=0.,k=5.;

   double  y = 1;

   auto  z =  y / s ; //<< --- ouch!

   std::cout << z << '\n';

}

@kwikius
Copy link
Contributor

kwikius commented Sep 15, 2020

Another advantage of constants relates to the N*M problem. The library only offers q_m_per_s and q_km_per_h, but maybe I want q_km_per_s. With constants, this isn't a problem.

In PQS I solved the problem of ad-hoc units by creating a so-called unit_binary_op that models the pqs::unit concept while providing customised output

Here is how an ad-hoc quantity can be created in source code
https://github.com/kwikius/pqs/blob/master/examples/fountain.cpp#L48

output :

PQS fountain power example
Demo of ad-hoc output units
spray height = 0.6 m
volume per s = 32.3202 cm³⁄s // <----------- Here is the custom output
mass per s = 32.3202 g⁄s // here is another ad-hoc unit
fountain output power = 0.190043 W

I show si units there but you can also use non-si /si combination of course and also stack them together

@JohelEGP
Copy link
Collaborator

There's also the case of linear algebra and other quantity wrappers.

using namespace units::physical::si;

fs_vector<si::length<si::metre>, 3> v = { 1_q_m, 2_q_m, 3_q_m };
fs_vector<si::length<si::metre>, 3> v = fs_vector<int, 3>{ 1, 2, 3 } * 1_q_m;
fs_vector<si::length<si::metre>, 3> v = fs_vector<int, 3>{ 1, 2, 3 } * m;
auto v = fs_vector<int, 3>{ 1, 2, 3 } * 1_q_m;
auto v = fs_vector<int, 3>{ 1, 2, 3 } * m;

si::length<si::metre, fs_vector<int, 3>> v(fs_vector<int, 3>{ 1, 2, 3 });

Do you think unit constants are an improvement here?

@JohelEGP
Copy link
Collaborator

A major practical problem with global constants is they can be hidden by local constants of the same name :

This can be mitigated by using the dimension concepts. Another reason to standardize them.

@mpusz mpusz unpinned this issue Dec 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed Polls
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants