mp-units: Effectively global unit names like 'm' are problematic

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. m is of course only one of many very short names which are brought in by the using namespace mp_units::si::unit_symbols declaration.

EDIT: for example :


#include <mp-units/systems/si/si.h>
#include <iostream>
#include <mp-units/format.h>
#include <mp-units/ostream.h>

using namespace mp_units::si::unit_symbols;

int main()
{
    auto q1 = 1 * m;

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

    int m = 1;

    auto q2 =  2 * m;

    std::cout << q2 <<'\n';
}

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.

#include <mp-units/systems/si/si.h>
#include <iostream>
#include <mp-units/format.h>
#include <mp-units/ostream.h>

namespace si_unit = mp_units::si::unit_symbols;

int main()
{
    static_assert(10.0 * si_unit::m / 2 == 5 * si_unit::m );

    auto q1 = 10.0f * si_unit::m;

    std::cout << q1 <<'\n';
    auto q2 = q1/q1;

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

}

Below is a small selection of the results of the search

~/ardupilot/libraries/SITL/SIM_Aircraft.cpp:436:            Matrix3f m = dcm;
~/ardupilot/libraries/SITL/SIM_Aircraft.cpp:437:            m = m * sitl->ahrs_rotation_inv;
~/ardupilot/libraries/SITL/SIM_XPlane.cpp:500:                    uint32_t m = uint32_t(data[7]) & j->mask;
~/ardupilot/libraries/AP_Declination/tests/test_magfield.cpp:119:        const Vector3f m = AP_Declination::get_earth_field_ga(loc);
~/ardupilot/libraries/AP_HAL_SITL/DSP.cpp:174:    uint16_t m = fft_log2(fftlen);
~/ardupilot/libraries/AP_Filesystem/AP_Filesystem_Mission.cpp:420:        mavlink_mission_item_int_t m {};
~/ardupilot/libraries/AP_Filesystem/AP_Filesystem_Mission.cpp:476:        mavlink_mission_item_int_t m {};
~/ardupilot/libraries/AP_Filesystem/AP_Filesystem_Mission.cpp:513:        mavlink_mission_item_int_t m {};
~/ardupilot/libraries/AP_Common/time.cpp:25:    m = 0;
~/ardupilot/libraries/AP_Common/time.cpp:34:            m = 0;
~/ardupilot/libraries/AP_RCTelemetry/AP_Spektrum_Telem.cpp:567:    m = 0;
~/ardupilot/libraries/AP_Math/tests/test_rotations.cpp:136:        vec2 = m * vec2;
~/ardupilot/libraries/AP_Math/examples/rotations/rotations.cpp:322:        vec2 = m * vec2;
~/ardupilot/libraries/AP_Math/examples/eulers/eulers.cpp:227:    v2 = m * v;
~/ardupilot/libraries/AP_Math/examples/eulers/eulers.cpp:233:    v2 = m * v;
~/ardupilot/libraries/AP_Math/examples/eulers/eulers.cpp:239:    v2 = m * v;
~/ardupilot/libraries/AP_Math/quaternion.cpp:416:    v = m * v;
~/ardupilot/libraries/AC_Avoidance/AP_OABendyRuler.cpp:697:        const float m = Vector3f::closest_distance_between_line_and_point(start_NEU, end_NEU, point_cm) * 0.01f - item.radius;

About this issue

Original URL
State: open
Created 4 months ago
Comments: 35 (32 by maintainers)

Most upvoted comments

We could consider an alternative approach here. Instead of having to do using namespace si::unit_symbols we could reorder the namespaces and provide using namespace unit_symbols::si. This would allow us to have a “safer mode”:
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 si::s and cgs::s.

The only downside here would be a bit harder definition of symbols as we would need to exit the mp_units::si namespace first in order to define them. And we would break our existing users as well… 😢

Promote the use of namespace si = mp_units::si::unit_symbols; instead.

JohelEGP on Mar 6, 2024

Ardupilot is a great example of using floats for everything and you spend a lot of time checking units .

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…

mpusz on Feb 27, 2024

You are going to have to write some quite dirty code, to move a legacy library from one to the other.

I am not sure what the ArduPilot code looks like. If it has some strong type wrappers (e.g., AP_Float), then the following chapter might be a good start for migration: https://mpusz.github.io/mp-units/latest/users_guide/use_cases/interoperability_with_other_libraries.

mpusz on Feb 27, 2024

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 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) https://github.com/ArduPilot/ardupilot/blob/master/libraries/AP_Soaring/Variometer.h

In comparison to

https://github.com/mpusz/mp-units/blob/master/example/glide_computer_lib/include/glide_computer_lib.h

kwikius on Feb 27, 2024

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;

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';

kwikius on Feb 27, 2024

q above will end up as the quantity of area 😢 as discussed here: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2024/p3045r0.html#potential-surprises-during-units-composition. It was safer before, but I got downvoted.

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.

kwikius on Feb 27, 2024

Compilation time is probably a discussion for another issue, so I should open that now

kwikius on Feb 26, 2024