Effectively global unit names like 'm' are problematic #558
Comments
|
I think the solution is to follow general C++ best practices, and avoid blanket using directives in favor of individual using declarations. That is, in each individual using mp_units::si::unit_symbols::m;...and similarly for every other symbol we want to incorporate. This has the advantage that it's easy to see where each symbol comes from, and of course also that it doesn't bring in a host of other symbols that we didn't think about. If we're in a file that has already used |
|
As stated in https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2024/p3045r0.html#constructing-a-quantity:
In case "importing"
In the V2 design, we have plenty of options to choose from. |
Maybe then best to rewrite the first example I came across to follow c++ best practice? |
The ctor syntax I would recommend is the quantity{value,unit} , but then I find myself thinking, I might just as well do it in a terser syntax: #include <mp-units/systems/si/si.h>
#include <iostream>
#include <mp-units/format.h>
#include <mp-units/ostream.h>
#define MAKE_QUANTITY(Quantity, Numeric, Unit, Mp_unit) \
namespace Quantity {\
struct Unit : decltype( mp_units::quantity{ Numeric() , Mp_unit })\
{\
explicit Unit( Numeric v) : quantity{v, Mp_unit }{}\
};\
}
namespace pqs {
MAKE_QUANTITY(velocity ,double ,m_per_s ,mp_units::si::metre/mp_units::si::second )
MAKE_QUANTITY(time ,double ,s ,mp_units::si::second )
}
int main()
{
auto q1 = pqs::velocity::m_per_s{20};
auto q2 = pqs::time::s{2};
pqs::time::s q3 = q2;
std::cout << q1 * q2 <<'\n';
std::cout << q3 <<'\n';
return 0;
} |
I do not think we have to rewrite the examples to use using-declarations. Using-directive is also fine in this case. In production code, we convert raw values to quantities in only a few places, typically only in a few functions. In such cases, it is often perfectly fine to use using-directives inside of such a function, and in case of naming conflicts, it doesn't hurt to spell the entire unit as we do not do this too often. In a production code, it is typically a mistake to use using-directives for the entire file scope, and we should nearly never do this inside of a header file. We have a lot of practice with An exception to this rule is typically unit tests, where we often create quantities from values. In such cases, using-directive is useful for the entire file as long as it does not produce hard-to-deal-with conflicts. |
I played a lot with the syntax you proposed in V1 and spoke about it on conferences as well (https://youtu.be/Nt7B1vI8DtI?si=W_loEqQkageu5dHT&t=2329). There a few big issues with it where the most important ones are:
Each of the syntaxes I described in the V1 had quite a lot of issues. I believe that the solution I came up for V2 is much better from anything that we had before. |
|
Composability of units https://github.com/kwikius/pqs/blob/master/examples/atmosphere.cpp#L17 and dimensions https://github.com/kwikius/pqs/blob/master/src/include/pqs/types/derived_quantity/acceleration.hpp#L9 were feature of pqs when mp-units V2 was a gleam in its authors eye I believe, with a similar syntax. I may be wrong about that but I seem to remember a variadic template and some heavyweight metaprogramming was required in mp-units V1 . If I got that wrong please correct me and I will apologise , so composability is not a problem here. But the thread is specifically about the issues arising from using effectively short global names to initialise a quantity which is a consequence of basically preventing the user from value initialising a quantity, unless supplying the units. For example the identifiers in |
There is no need to apologize. We are not arguing or fighting here. We all want to find the best solution, and we need to discuss different approaches. Said that, I do not agree with that statement. How do you achieve
The usage of short global names has nothing in common with preventing the user from value initializing a quantity. Those are totally orthogonal. We would use the latter no matter of the way we spell the unit. The latter is about safety, and we believe it is the right way to go. Plenty of people (including the ISO C++ Committee experts) warned us about the safety issues that arise from the similar approach in
Yes, we will need to optimize compile times at some point. Until now, I have not spent any time on improving those as we still struggle to address the last missing pieces of the design, which have priority now. This library was designed from the very beginning with the C++ modules in mind, and support for C++ modules is arriving just now. Hopefully, soon, we will have some time and the possibility to work on the compile time performance.
This is why we describe this in one of the first chapters of your documentation: https://mpusz.github.io/mp-units/latest/getting_started/quick_start/#quantities. We may extend that admonition with more details if you think it is needed. |
|
I've just extended the documentation here: https://mpusz.github.io/mp-units/latest/getting_started/quick_start/#quantities. |
|
A side note on the compilation performance It is true that we can optimize the compilation performance, and we will do it for sure at some time in the future. However, usage examples compile so slow because of several additional reasons:
|
It is a good example, but again with short unit names particularly it is not infallible #include <mp-units/systems/si/si.h>
#include <iostream>
#include <mp-units/format.h>
#include <mp-units/ostream.h>
#include <vector>
using namespace mp_units::si::unit_symbols;
namespace si = mp_units::si;
int main()
{
auto constexpr q1 = 1 * m;
auto constexpr m = q1 - q1;
std::vector< mp_units::quantity<si::metre> > v;
v.emplace_back( 42 * m );
std::cout << q1 / v[0] <<'\n';
return 0;
} |
|
Sure! |
|
We could not provide those at all and leave the definition of short names to the user. It would even improve compile times a lot as this is the biggest header file to process right now (https://github.com/mpusz/mp-units/blob/master/src/systems/include/mp-units/systems/si/unit_symbols.h). However, there are many places where they can be used and improve the usability and readability of code. This is why we decided to provide them with the library. |
|
Compilation time is probably a discussion for another issue, so I should open that now |
|
Sure, please do. Although it may take a while before we start tuning up the compile-time performance. |
|
Is there anything else you would like to discuss in this issue? Can we improve something here besides the docs that I've updated already? |
|
I think the green "Important" block should be improved, yes. We currently list the "using directive" approach first, and without comment. This makes it look like we're recommending it! I think we should instead list it last, and explicitly say that we don't recommend the approach in production code, and give a couple of reasons why. Another big advantage of unit symbols over user-defined literals is that we no longer need to recommend the using directive approach. |
|
I would basically concur with @chiphogg . When I start reading the documentation I would like to have those short names put in some sort of context , and show intended best practice at least at first encounter in the documentation. Later perhaps this could be abbreviated, though every page is an entry point for somebody. I modified the code to get it to work with gcc. The anonymous namespace is intended to show file scope so the units could be used in other functions in the source file.. YMMV #include <format>
#include <iomanip>
#include <iostream>
#include <mp-units/format.h>
#include <mp-units/ostream.h>
#include <mp-units/systems/si/si.h>
#include <mp-units/systems/international/international.h>
using namespace mp_units;
constexpr QuantityOf<isq::speed> auto avg_speed(QuantityOf<isq::length> auto d,
QuantityOf<isq::time> auto t)
{
return d / t;
}
namespace {
// comments here
using si::unit_symbols::km;
using si::unit_symbols::m;
using si::unit_symbols::h;
using si::unit_symbols::s;
// etc
using international::unit_symbols::mph;
//etc
using international::unit_symbols::mi;
}
int main()
{
constexpr quantity v1 = {110 , km / h};
constexpr quantity v2 = 70 * mph; // alternate form
constexpr quantity v3 = avg_speed(quantity{220., isq::distance[km]}, quantity{2 , h});
constexpr quantity v4 = avg_speed(isq::distance(140. * mi), 2 * h); // alternate form
constexpr quantity v5 = v3.in(m / s);
constexpr quantity v6 = value_cast<m / s>(v4);
constexpr quantity v7 = value_cast<int>(v6);
std::cout << v1 << '\n'; // 110 km/h
std::cout << std::setw(10) << std::setfill('*') << v2 << '\n'; // ***70 mi/h
std::cout << std::format("{:*^10}\n", v3) <<'\n'; // *110 km/h*
std::cout << std::format("{:%N in %U of %D}", v4)<<'\n'; // 70 in mi/h of LT⁻¹
std::cout << std::format("{:{%N:.2f}%?%U}", v5)<<'\n'; // 30.56 m/s
std::cout << std::format("{:{%N:.2f}%?{%U:dn}}", v6)<<'\n'; // 31.29 m⋅s⁻¹
std::cout << std::format("{:%N}", v7)<<'\n'; // 31
} |
|
Agreed, I will review the docs and think about how to improve the examples. However, I do not think that there is anything wrong with using-directives if they are used in a limited (e.g., function) scope. So, in the example above, it is perfectly fine to add those to the I even think that bringing them to the namespace is as bad as similar usage of using-directives as they may collide in some unforeseen places anyway. Raw numbers are (or at least should be) converted to quantities only in a few specific places in the project, and having unit symbols accessible in the entire program through its namespace is encouraging to do wrong things as well and may easily backfire. |
|
I just finished the review of all the examples in the documentation. Using-directive was nearly never used in the global/namespace scope, so we are fine. Additionally, I improved the "Important" admonition in the docs to warn against the usage of using-directives in a wide scope. |
In the above example I used specifically an anonymous (unnamed) namespace , which emphasises that the names have internal linkage, so are only available in the particular translation unit, they are defined in. but the example here is enough for me to recommend avoiding this style as far as possible. Bringing the entire unit short name set into a large function is arguably as bad. You have the opposite problem. One day a new unit may be added to the namespace and quietly hide a value from an outer scope FWIW I don't know if this is correct behaviour in the following (gcc-13) , but if you uncomment the SOMETHING define assert fires, and not if you don't. And this is why I probably will recommend not to use any of these short unit aliases! #include <format>
#include <iomanip>
#include <iostream>
#include <mp-units/format.h>
#include <mp-units/ostream.h>
#include <mp-units/systems/si/si.h>
#include <mp-units/systems/international/international.h>
#include <cassert>
using namespace mp_units;
#define SOMETHING
void f(){
//...
#if defined SOMETHING
quantity km = { 1.e-3, si::metre};
#endif
//...
{
using namespace si::unit_symbols;
auto q2 = 10 * km;
assert( q2.unit == si::kilo<si::metre>);
}
}
int main()
{
f();
}
|
|
FWIW in pqs a unit is composable using op * , op / , etc which returns a unit but there is no multiplication of a unit by a number. in using mp-units I would recommend always using the mp-units value constructor that keeps the number and the unit separate: auto q = quantity{number, unit};Interestingly, in #558 (comment) if I replace with I get an error message when SOMETHING is defined and that seems to be a big advance EDIT: In fact if I got around to reworking pqs, I think that {number, unit} is the value ctor I would opt for. It allows composability of math.numbers and of physical.units while keeping them in their separate domains, and the quantity constructor is a 'black box' which unites the domain of maths and physics in a quantity system |
Sure, this is a correct behavior. If you define a local variable
Sure, if you want to do non-trivial logic in your function, it is perfectly fine not to use symbols and stick with full unit names. Also, you probably should not recommend using auto v = 10 * si::kilo<si::metre>;
auto q = v * si::kilo<si::metre>;
|
|
We could consider an alternative approach here. Instead of having to do using namespace unit_symbols;
auto q = 42 * si::m;which would be much harder to hijack. Also, this would allow us to easier disambiguate between The only downside here would be a bit harder definition of symbols as we would need to exit the |
I don't want to have to be clever and trip up my users and then tell them how dumb they are for not knowing the intricate rules at work here. I consider myself a reasonably competent C++ programmer and I didn't know the rules here. |
Sure , units shouldn't be mixed with numbers. They are different domains. This is the best approach for clarity. auto speed_unit = mp_units::si::metre / mp_units::si::second;
mp_units::quantity q = {20.0f, speed_unit};
float value = q.numerical_value_in(q.unit);
std::cout << value << '\n';
auto area_unit = mp_units::si::metre * mp_units::si::metre;
mp_units::quantity q1 = {10, area_unit};
value = q1.numerical_value_in(q1.unit);
std::cout << value << '\n';
|
This probably is not going to happen because the constructor syntax does not compose. Just compare the two definitions: quantity q1 = quantity{7, non_si::hour} + quantity{20, non_si::minute} + quantity{35, si::second};
quantity q2 = 7 * h + 20 * min + 35 * s;The second one is so much easier to use. However, as it is clearly stated in our docs (https://mpusz.github.io/mp-units/latest/getting_started/quick_start/#quantities), the multiply syntax is not mandatory, and explicit constructor call may be used instead. The same applies to unit_symbols, which are not provided by default. Users have the choice to decide which style best suits their needs. |
Yes. All the problems there are due to being able to multiply numbers ( and quantities) by units. Neither quantities nor number should be composable with units, I think the practise and syntax started with Walter Browns SI units library. https://lss.fnal.gov/archive/1998/conf/Conf-98-328.pdf . Anyway, there is an alternative value ctor provided so I will try using that exclusively and see how that goes. |
|
I would actually be really interested in hearing some feedback from you when you are done. My understanding is that conversions from unsafe raw values to quantities should happen only in a few entry points to a program, and those should be limited to a very narrow scope to prevent potential safety issues. Mixing unsafe raw quantity values and strong quantities in the same context might be a recipe for disaster. |
You are being somewhat unfair to the 2 arg ctor here. Short names for units would be much more acceptable if they didnt compose with numbers and quantities so what is good for the goose is good for the gander // may be a better way to do this
auto q = []( auto v, auto U) { return quantity {v, U };};
quantity q1 = q(7, h) + q(20, min) + q(35, s);
quantity q2 = 7 * h + 20 * min + 35 * s;
std::cout << " q1 = " << q1 <<'\n';
std::cout << " q2 = " << q2 <<'\n'; |
You are going to have to write some quite dirty code, to move a legacy library from one to the other. Ardupilot is a great example of using floats for everything and you spend a lot of time checking units . ( Bear in mind ArduPilot is huge and I suspect there would be a lot of resistance to change) Here is one part that might be of interest to you . ( Ardupilot can do autonomous thermal soaring) In comparison to |
|
The killer application for mp-units and ardupilot would be if use of units would increase speed and reduce code size. I am pretty sure that it can FWIW. There are piecemeal efforts to streamline units e.g ArduPilot/ardupilot#25768 . Ardupilot uses degrees, centidegrees and radians pretty much randomly throught out the codebase. |
I am not sure what the ArduPilot code looks like. If it has some strong type wrappers (e.g., |
Yes, I totally understand the issue, as I contributed for a few years to the http://lk8000.it. It was 15 years ago, and this is when I realized how important it is to solve the issue of units in similar code bases. I work on it from then... |
Promote the use of |
The use of names such as 'm' effectively in the global namespace doesn't scale well. I have been looking into trying to use mp-units in a real library and selected ArduPilot, since I know it well. I did a search for ' m ' in the codebase in .cpp and .h files and came up with around 350 hits.
mis of course only one of many very short names which are brought in by theusing namespace mp_units::si::unit_symbolsdeclaration.EDIT: for example :
The rough solution I came up with was a namespace alias, but is there another official alternative way to initialise quantities than these very unit names, because I can see them being very problematic if you are trying to update an existing codebase and this is the only way to initialise a quantity.
Below is a small selection of the results of the search
The text was updated successfully, but these errors were encountered: