Some remarks and issues

[Lecrapouille]

Hi ! Nice API that seems more user-friendly than boost. I'm testing it.

1/ Could be nice to add in your documentation ::value() how to cast a SI type to a type such a float. This is somewhat needed. I had to find it back from the code source.

auto a = 1.5_km;
std::cout << a.value() << std::endl;

Why not to add something like this ::to<T>() { return T(value()); } ? for example, a.to<int>() will return 1.

2/ Why degree_t is missing and how to write code such as 1.0_rad + 1.0_deg or degree_t d = 1.0_rad ? I tried this but does not compile:

using degree_t = SI::radian_t<double, std::ratio<10000, 572958>>;
degree_t a = 1.0_rad;
std::cout << a.value() << std::endl;

But only this odd code works:

using degree_t = SI::velocity_t<double, std::ratio<10000, 572958>>;
degree_t a = 1.0_m_p_s;
std::cout << a.value() << std::endl;

3/ You have types such as velocity and acceleration, we have to include the specific header files, but you have not jerk, jounce, crackle, pop (https://en.wikipedia.org/wiki/Fourth,_fifth,_and_sixth_derivatives_of_position), maybe is there a more generic way to generate them?

4/ Why I cannot display velocities ? The follow code

#include <SI/length.h>
#include <SI/time.h>
#include <SI/velocity.h>
#include <SI/stream.h>

auto a = 1.0_km / 1.0_s;
std::cout << a << std::endl;

will produce this error:

/usr/local/include/SI/stream.h: In instantiation of ‘std::ostream& operator<<(std::ostream&, const SI::detail::unit_t<_symbol, _exponent, _type, _ratio>&) [with char _symbol = 'v'; _exponent = std::ratio<1>; _type = long double; _ratio = std::ratio<1000, 1>; std::ostream = std::basic_ostream<char>]’:
main.cpp:17:17:   required from here
/usr/local/include/SI/stream.h:23:26: error: ‘str’ is not a member of ‘SI::unit_symbol<'v', std::ratio<1000, 1>, std::ratio<1> >’
   stream << unit.value() << SI::unit_symbol<_symbol, _ratio, _exponent>::str;

Have I missed an include (same for acceleration) ?

5/ This code works:

SI::metre_t<float> a = 1.5_km;
SI::seconds_t<float> s = 1.0_s;
SI::metre_per_second_t<float> c = a / s;

But this one fails to compile (because of the double to float conversion):

SI::metre_t<float> a = 1.5_km;
SI::metre_per_second_t<float> c = a / 1.0_s;

6/ I'm on the HEAD and not have conan but compilation will fail

 cmake ..
-- The CXX compiler identification is GNU 8.3.0
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Warning at test/CMakeLists.txt:24 (message):
  Conan not found.  Trying to find Catch2 without it.

-- Found Doxygen: /usr/local/bin/doxygen (found version "1.8.17 (d4a0825b1d00ead2ce441c3407d1983f80401353*)") found components: doxygen dot 
-- Configuring done
-- Generating done
-- Build files have been written to: /home/qq/SI/build

Then:

/home/qq/SI/test/src/benchmarks/unit_assignment_benchmarks.cc: In lambda function:
/home/qq/SI/test/src/benchmarks/unit_assignment_benchmarks.cc:18:37: error: ‘storage_for’ is not a member of ‘Catch::Benchmark’
       std::vector<Catch::Benchmark::storage_for<ratio_one_unit>> storage(

[bernedom]

Hi @Lecrapouille,

Thanks for raising the issues. I only found time to look at them now, but here are some answers.

1) - This really seems to be missing from the documentation. adding a to<T>() function looks like a good idea. I will take it up for the next release

2) degree_t is missing, because it is not a SI unit. Also "degree" is pretty ambigous because it could be confusing with temperature (Kelvin, celsius, fahrenheit...) I know that I already added some non-SI units, so I might consider adding them, in the future. To make your code compile use using degree_t = SI::angle_t<double, std::ratio<10000, 572958>>; (note the angle_t instead of radian_t. Generally to create new units one has to derive from the dimension-type.

3) Again jerk, jounce, crackle, pop are not SI units. I have to think about adding them.

4) Seems I forgot to add the streaming capabilities for velocity. I will fix this in the next release.

5) I have to check what is missing here. Should actually work

6) which version of catch2 do you have? The benchmarking support only came in catch2 2.9

[Lecrapouille]

Hi ! All your points make sense. I'll try degree_t For the version of catch2 I'm not sure there is a catch2 package for Debian 10

[Lecrapouille]

Yesterday I did not have a lot of time to replied you, here is a longer reply:

• For 4. "Seems I forgot to add the streaming capabilities for velocity."

  • from my memory, acceleration is also impacted.

• For 2. I know that I already added some non-SI units

  • According to wikipedia There is a special group of units that are called "non-SI units that are accepted for use with the SI" https://en.wikipedia.org/wiki/Non-SI_units_mentioned_in_the_SI it's totally fine if the lib only deals with "pure" SI and let the user to deal by itself with no-SI needing a conversion factor, but this could be nice to add a section in your documentation on how to acheive it.
  • If I tested your lib is because I have lot of deg to rad to convert: std::cos() in rad but a graphical lib using deg for their rotations :(

• For 3. when you are saying jerk, jounce, crackle, pop are not SI.

  • I'm not expert, so maybe I'm wrong but, should not we consider that when we are operating on two SI (operators such as + * and their opposite) the result should also be an SI ? For example, [velocity] = [metre] / [time] and [acceleration] = [velocity] / [time] = [metre] / [time] / [time] all are made of atomic SI. But I'm not feeling comfortable with this assertion since that can be said we can mix for example [Luminance] with [Force] which probably has not meaning.

• For 5. in the 1st example a and s are two float, while in the second example 1.0_s is a double and not sure if we can write 1.0f_s for float. Strong type is fine for me since I usually add to my g++ some flags (I don't remember their names) checking implicit casts and loose of resolution (ie uint8_t a := uint32_t x).

• For 6. not having unit tests working for me is minor (debian 10 amd64).

[bernedom]

Issue 5) is fixed in release 2.1.3. You can now mix units with different internal types, as long as they are implicitly castable. You might still get compiler warnings etc. about the loss of precision if done the wrong way, but the missing operators are now there.

[Lecrapouille]

I'm converting slowly my application by adding SI. Because of external lib not using strong type I have to use many .value() which add noise for the reading. Cannot we allow implicit cast from SI to float, double with the operator= ?

For example:

SI::metre_t<float> m = 5.0_m;
float v = m; // should be allowed instead of v = m.value() ?
m = 4.0f; // still forbidden of course

For angles, I made something like this (ok here Degree is for angle only since I'm not concerned by temperature):

template<class T>
using degree_t = SI::angle_t<T, std::ratio<10000, 572958>>;

using Degree = degree_t<float>;
using Radian = SI::angle_t<float, std::ratio<1>>;

constexpr degree_t<long double> operator""_deg(long double value)
{
    return degree_t<long double>{value};
}

constexpr float cos(Radian const a)
{
   return ::cosf(a.value());
}

constexpr float sin(Radian const a)
{
   return ::sinf(a.value());
}

constexpr float cos(Degree const d)
{
   return ::cosf(Radian(d));
}

constexpr float sin(Degree const d)
{
   return ::sinf(Radian(d));
}

This code will compile:

Degree d = 45.0_deg;
std::cout << cos(d) << std::endl;

But not this one:

std::cout << cos(45.0_deg) << std::endl;

because of confusion between rad and deg. That's why maybe implict conversion to C++ primitive unit could solve this.

[bernedom]

Implicit conversion to the internal type has the problem that it causes ambiguity with the operators that take plain scalars, like operator*(_type) and the built in operators of C++.

i.e. a call to auto r = 1_m * 1000) could either be translated to int64 r = nt64 * int64 if 1_m1 is first implicitly converted to int64 or it could be metre_t<int64> r = metre_t<int64> * int64 which would be what is intended.

What I can do is adding explicit conversion operators, but that still means that you have to write that cast manually.

As for your example, I think this std::cout << cos(45.0_deg) << std::endl; fails, because _deg will return an angle_t<long double> and your Degree is an angle_t<float> and there is also not yet an implicit conversion for this available. I will see if I can figure out how to add this.

So to make things short, use calls to value() if you need to access the scalar within directly.

[Lecrapouille]

note: to be clear I was talking about this operator https://en.cppreference.com/w/cpp/language/cast_operator another possibility was the operator()() instead of value() but I'm not a big fan of this notation auto r = 1_m; long double val = r(); at least () is 2 char to type while .value() is 8 char ^^.

You are certainly right! When you said operator*(_type) you mean https://en.cppreference.com/w/cpp/language/operator_arithmetic right? Times operator with scalar should be allowed while forbidden for addition but for auto should not SFINAE (Substitution Failure Is Not An Erro) suppose to help us for auto? I mean int64 r = 1_m * 1000 shall fall, so metre_t r = 1_m * 1000 is deduced ? By the way, this is why I never use it (except for iterating on containers to have shorter code), for joking: in my opinion, auto was introduced as an intermediate step before fully converting C++ to Javascript (the next step will be to remove the keyword auto) ^^

[bernedom]

note: to be clear I was talking about this operator https://en.cppreference.com/w/cpp/language/cast_operator another possibility was the operator()() instead of value() but I'm not a big fan of this notation auto r = 1_m; long double val = r(); at least () is 2 char to type while .value() is 8 char ^^.

I will not add the operator()() because, in my opinion, it is smell.
I tried to add the cast-operator. It works of the cast operator is marked explicit but the tests do no longer compile if the explicit is removed, because of the ambiguity mentioned in the reply above: https://github.com/bernedom/SI/issues/93#issuecomment-925336981

explicit constexpr operator _type() const { return value_; }
explicit constexpr operator _type &() { return value_; }

Edit: See this branch, to try it out: https://github.com/bernedom/SI/tree/feature/implicit-cast-to-primitive

[Lecrapouille]

Thanks, sorry for the delay, currently I have few time for testing.

[bernedom]

Release 2.2.0 contains degree_t and the literal _deg to create degrees. They can be converted to radians and vice versa. so a value of 1 in radians are ~57°

[bernedom]

version 2.3.0 will contain string conversions for velocity types. This issue will be closed once the release is out