mpusz / mp-units

The quantities and units library for C++
https://mpusz.github.io/mp-units/
MIT License
998 stars 79 forks source link

problems casting (angle) type to itself #447

Closed ka-ba closed 8 months ago

ka-ba commented 1 year ago

This is my problem, first described in a discussion thread, formulated as proper issue, like proposed in https://github.com/mpusz/units/discussions/428#discussioncomment-5470934 .

I created two types that represent angles with a custom representation base class each (for the different units radian and degree). While being able to quantity_cast one into the other in both directions, the code fails to quantiy_cast rad into rad or degree into degree. (Pls. note that the problem arises with a function that accepts any angle type, but internally converts to radian before starting computations (which is my implementation of sin, also to be found below).)

Pls find a down-sized example of the problem here: https://godbolt.org/z/c58P97hzx (the complete code also follows at the end of this text for reference) (The compiler is set to gcc11.3 as that is my local version, but the recent versions of gcc and clang complain just alike.)

As quantity_casting other units types to themselves works AFAI can see, there seems to be a problem with my use of the library. At this stage I won't suspect an issue within the library, but any help with it's proper usage is very welcome.

The example code that can also be found in compiler explorer (link above) (sin provided for the sake of completeness):

#include <iostream>
#include <units/generic/angle.h>

struct radian_base_t final {
    using value_type = double;
    value_type value{};
    constexpr radian_base_t() = default;
    constexpr /*explicit*/ radian_base_t(double const r_ad) : value(r_ad>std::numbers::pi*2. ? std::remainder(r_ad,std::numbers::pi*2.) : r_ad) {}
    auto operator<=>(radian_base_t const&) const = default;
    constexpr radian_base_t operator*(double const rhs) const { return radian_base_t{value*rhs};}
    constexpr radian_base_t operator/(double const rhs) const { return radian_base_t{value/rhs};}
    constexpr radian_base_t operator-(radian_base_t const rhs) const { return radian_base_t{value-rhs.value};}
    constexpr radian_base_t operator+(radian_base_t const rhs) const { return radian_base_t{value+rhs.value};}
    constexpr operator double() const { return value; }
};
using angle_t = units::angle<units::radian,radian_base_t>;

constexpr auto MULTIPLIER = std::numeric_limits<std::intmax_t>::max()/200;
constexpr auto MULTIPLEPI = static_cast<std::intmax_t>(std::numbers::pi_v<long double>*MULTIPLIER);
constexpr auto MULTIPLE180 = MULTIPLIER*180;
namespace units {
struct deg : named_scaled_unit<deg, "deg", no_prefix, ratio(MULTIPLEPI, MULTIPLE180), radian> {};
}
struct degree_base_t final {
    using value_type = double;
    value_type value{};
    constexpr degree_base_t() = default;
    constexpr /*explicit*/ degree_base_t(double const r_ad) : value(r_ad>360 ? std::remainder(r_ad,360) : r_ad) {}
    auto operator<=>(degree_base_t const&) const = default;
    constexpr degree_base_t operator*(double const rhs) const { return {value*rhs};}
    constexpr degree_base_t operator/(double const rhs) const { return {value/rhs};}
    constexpr degree_base_t operator-(degree_base_t const rhs) const { return {value-rhs.value};}
    constexpr degree_base_t operator+(degree_base_t const rhs) const { return {value+rhs.value};}
    constexpr operator double() const { return value; }
};
using deg_t = units::angle<units::deg,degree_base_t>;

int main(int,char**) {
    angle_t rad_orig{3.141562};
    auto rad2deg = units::quantity_cast<deg_t>(rad_orig);
    std::cout << rad2deg.number() << "\n";
    auto deg2rad = units::quantity_cast<angle_t>(rad2deg);
    auto rad2rad = units::quantity_cast<angle_t>(rad_orig);
    // auto deg2deg = units::quantity_cast<deg_t>(rad2deg); comparable error
}

template<typename D, typename U, typename Rep>
requires units::Angle<quantity<D,U,Rep>>
[[nodiscard]] inline units::dimensionless<units::one> sin(quantity<D, U, Rep> const &q) noexcept
  requires requires { std::sin(q.number())/*std::sin(units::quantity_cast<angle_t>(q).number())*/; } {
    return {std::sin(units::quantity_cast<angle_t>(q).number())};
}

(EDIT: Just realised that the error message is missing here...) The error message that g++ produces on compiler explorer:

In file included from <source>:2:
In file included from /opt/compiler-explorer/libs/mp-units/v0.7.0/src/core/include/units/generic/angle.h:26:
In file included from /opt/compiler-explorer/libs/mp-units/v0.7.0/src/core/include/units/quantity.h:26:
In file included from /opt/compiler-explorer/libs/mp-units/v0.7.0/src/core/include/units/bits/common_quantity.h:27:
/opt/compiler-explorer/libs/mp-units/v0.7.0/src/core/include/units/quantity_cast.h:119:31: error: implicit instantiation of undefined template 'units::detail::cast_traits<radian_base_t, radian_base_t>'
  using ratio_type = TYPENAME traits::ratio_type;
                              ^
<source>:43:27: note: in instantiation of function template specialization 'units::quantity_cast<units::quantity<units::dim_angle<>, units::radian, radian_base_t>, units::dim_angle<>, units::radian, radian_base_t>' requested here
    auto rad2rad = units::quantity_cast<angle_t>(rad_orig);
                          ^
/opt/compiler-explorer/libs/mp-units/v0.7.0/src/core/include/units/quantity_cast.h:81:8: note: template is declared here
struct cast_traits;
       ^
/opt/compiler-explorer/libs/mp-units/v0.7.0/src/core/include/units/quantity_cast.h:120:29: error: implicit instantiation of undefined template 'units::detail::cast_traits<radian_base_t, radian_base_t>'
  using rep_type = TYPENAME traits::rep_type;
                            ^
/opt/compiler-explorer/libs/mp-units/v0.7.0/src/core/include/units/quantity_cast.h:81:8: note: template is declared here
struct cast_traits;
       ^
<source>:48:23: error: use of undeclared identifier 'quantity'
requires units::Angle<quantity<D,U,Rep>>
                      ^
3 errors generated.
Execution build compiler returned: 1
mpusz commented 10 months ago

I am so sorry that it took me so long to come back to you. I was working hard on the V2 version of the engine, and only now is it ready enough to actually show you how to solve your issue in this library.

Here it is https://godbolt.org/z/vMcdxzEY5. When we are dealing with custom representation types, we need to describe their semantics with:

Moreover, for your example to compile, you also needed a converting constructor from one type to another (radian_rep_t -> degree_rep_t, degree_rep_t -> radian_rep_t) so the underlying library framework knows how to initialize them correctly from the function arguments.

Also, as now they are floating-point-like you do not need any explicit casts anymore to convert between those.

Sorry again for the late answer. Please let me know if I can close the issue or if there are some other issues that we need to resolve first.

ka-ba commented 9 months ago

Thanks a lot for your answer! I'm currently working on another branch (and just in spare time). I'll try to have a look at the above solution soon. It's just a small project, but it would be nice to use the units lib and have all type interaction checked at compile time.

mpusz commented 9 months ago

Sure, please let me know if your problem is fixed, and we can close this issue.

ka-ba commented 8 months ago

Sorry for the delay - had quite a hard time converting anything from 0.7-syntax to v2-syntax (even more so since the IDE uses clangd for problem highlighting and clang seems to have known bug(s) concerning concepts :frowning_face: ). Everything compiles fine (using gcc), but I didn't do a lot of testing. As for angular measures: the two quantities compile well, especially the conversion between them - as per your example. Thanks again for that! But to me it seems that the resulting value is incorrect!? Having an angle of PI rad, it should print something around 180°, shouldn't it? (c.f. your godbolt-link above) I can't see a respective bug in the base classes. Do you have an idea here as well?

(NB: In case you are interested, the commit with the changes from v0.7 -> v2.0 : https://gitlab.com/ka-ba/vfrp/-/commit/b4a5bac06b50faec4eb7050820e5f4a8b41bdb81 , sorry that renaming one type clutters the diffs)

mpusz commented 8 months ago

@ka-ba, thanks for sharing your code. I left some comments in your commit.

Regarding your question, the problem is that your types are implicitly convertible from double, which results in radian_base_t being selected as the type used to do all the internal conversions in the library. They are also implicitly convertible to each other, which does not help as well. I am not sure why you need the other constructor. If it is needed, it should be explicit as well. If not, just remove it. Also, custom overloads of arithmetic operators seem to not be needed as the implicit conversion to double will provide them anyway.

With the above changes the code works as expected: https://godbolt.org/z/Y443b7xec.

mpusz commented 8 months ago

@ka-ba, please let us know if the above addresses your problems and if we can close this issue.

ka-ba commented 8 months ago

@mpusz Thanks again for your help with the library! The arithmetic operators and the implicit constructor from double also had other usages in my code. As implicit construction causes severe problems, I have to do without, of course. I found work-arounds (though not very nice looking). It doesn't seem necessary to reach for clumsy work-arounds concerning arithmetic, so I'll keep the operators with the base_classes. If of interest for you, pls see https://godbolt.org/z/5c8rdKd7W for details.

The original problem is solved, I'd say - the issue should be closed. Thanks again for your help and for the library as such! I hope it's going to be used in many projects.

P.S.: Also thanks a lot for commenting my project. Taking care of your comments will be my next task.