johnmcfarlane / cnl

A Compositional Numeric Library for C++
Boost Software License 1.0
644 stars 62 forks source link

How to use as scaled decimal number (1.x) #991

Open vrqq opened 2 years ago

vrqq commented 2 years ago

When I try to use this code below, the complier error occurred when std::cout.

using DecX = cnl::scaled_integer<cnl::rounding_integer<long long>, cnl::power<-1, 100>>;
DecX num1 = 10.554;
DecX num2 = 10.545;
std::cout<<num1<<" "<<num2<<std::endl;

Clang 12 with libstdc++ and c++ 17

In file included from ../third/cnl-1.1.7/include/cnl/_impl/scaled_integer/extras.h:22:
../third/cnl-1.1.7/include/cnl/_impl/scaled_integer/to_chars.h:52:24: error: no matching function for call to 'scale'
                return scale<Exponent, Radix>(to_rep(scalar));
                       ^~~~~~~~~~~~~~~~~~~~~~
../third/cnl-1.1.7/include/cnl/_impl/scaled_integer/to_chars.h:66:48: note: in instantiation of member function 'cnl::_impl::split<cnl::_impl::number<long long, cnl::native_rounding_tag>, -1, 100, false>::integral' requested here
                return from_integral_and_value(integral(value), value);
                                               ^
../third/cnl-1.1.7/include/cnl/_impl/scaled_integer/to_chars.h:207:32: note: in instantiation of member function 'cnl::_impl::split<cnl::_impl::number<long long, cnl::native_rounding_tag>, -1, 100, false>::operator()' requested here
            auto const split = _impl::split<Rep, Exponent, Radix>{}(value);
                               ^
../third/cnl-1.1.7/include/cnl/_impl/scaled_integer/../to_chars.h:88:28: note: in instantiation of function template specialization 'cnl::_impl::to_chars_positive<cnl::_impl::number<long long, cnl::native_rounding_tag>, -1, 100>' requested here
                    return to_chars_positive(first, last, value);
                           ^
../third/cnl-1.1.7/include/cnl/_impl/scaled_integer/to_chars.h:242:16: note: in instantiation of member function 'cnl::_impl::to_chars_non_zero<cnl::scaled_integer<cnl::_impl::number<long long, cnl::native_rounding_tag>, cnl::power<-1, 100>>, true>::operator()' requested here
        return _impl::to_chars_non_zero<native_rounding_type>{}(
               ^
../third/cnl-1.1.7/include/cnl/_impl/scaled_integer/../to_chars.h:120:23: note: in instantiation of function template specialization 'cnl::to_chars<cnl::_impl::number<long long, cnl::nearest_rounding_tag>, -1, 100>' requested here
        auto result = to_chars(chars.data(), chars.data()+max_num_chars, value);
                      ^
../third/cnl-1.1.7/include/cnl/_impl/scaled_integer/extras.h:210:23: note: in instantiation of function template specialization 'cnl::to_chars<cnl::scaled_integer<cnl::_impl::number<long long, cnl::nearest_rounding_tag>, cnl::power<-1, 100>>>' requested here
        return out << to_chars(fp).data();
                      ^
../study/try_cnl.cpp:43:9: note: in instantiation of function template specialization 'cnl::operator<<<cnl::_impl::number<long long, cnl::nearest_rounding_tag>, -1, 100>' requested here
    cout<<num1<<" "<<num2<<endl;
        ^
../third/cnl-1.1.7/include/cnl/_impl/number/../num_traits/scale.h:52:38: note: candidate template ignored: substitution failure [with Digits = -1, Radix = 100, S = cnl::_impl::number<long long, cnl::native_rounding_tag>]: implicit instantiation of undefined template 'cnl::scale<-1, 100, cnl::_impl::number<long long, cnl::native_rounding_tag>>'
        [[nodiscard]] constexpr auto scale(S const& s)
                                     ^
1 warning and 2 errors generated.

Then I check the source code at: cnl/rounding_integer.h:63 I am confusing that why can't be default_scale<(Digits < 0), (Radix != 2)>? (tag v1.7, also v1.x branch currently)

https://github.com/johnmcfarlane/cnl/blob/2dde6e62e608a4adc3c5504f067575efa4910568/include/cnl/rounding_integer.h#L57-L73

johnmcfarlane commented 2 years ago

Hi. Thanks for filing this issue. It has since been addressed in the latest version: 1.x: https://godbolt.org/z/1Y53M5Knj 2.x: https://godbolt.org/z/exxafsKjx Is it possible for you to upgrade?

I am confusing that why can't be default_scale<(Digits < 0), (Radix != 2)>?

The specialisation that was provided in v1 is likely to be using a bit shift operation and so only works for the binary base.

vrqq commented 2 years ago

Still in v1 since the C++17 we used. I try to add these code in cnl/_impl/num_traits/scale.h ( https://github.com/johnmcfarlane/cnl/blob/v1.x/include/cnl/_impl/num_traits/scale.h#L48 ) And then the problem solved.

namespace cnl {
    template<int Digits, int Radix, template<typename, typename> class TNUM, typename S, typename Tag>
    struct scale<Digits, Radix, TNUM<S, Tag>, _impl::enable_if_t<(
        Digits<0 && cnl::_impl::is_integral<S>::value)>>
    {
        CNL_NODISCARD constexpr auto operator()(TNUM<S, Tag> const& s) const
        -> decltype(s/_impl::power_value<S, -Digits, Radix>())
        {
            return s/_impl::power_value<S, -Digits, Radix>();
        }
    };
}

Could you please help me to review these code?

johnmcfarlane commented 2 years ago

Hi. Thanks for the feedback and suggestion. I should be able to take a look at it later today.

The great thing about DVCS and CI is that you can safely break things without causing harm or disruption to others. So while I'm happy to help, you don't have to wait.

  1. fork the project under your own user account,
  2. See whether GitHub Actions runs and builds the project for you (it may not or there may have been degradations in the build system over the past few months; I'm not sure),
  3. make a branch off of v1.x,
  4. add a test which fails - it doesn't have to be perfect or ready for merging but shows the problem,
  5. apply the change,
  6. push the branch to GitHub,
  7. see whether the fix prevents the test from failing.
johnmcfarlane commented 2 years ago

CI for v1.x is not currently working, likely due to updates to the docker images used to build the project. I'll need to fix the v1.x branch first.

johnmcfarlane commented 2 years ago

Test case pushed and confirmed.

Fix pushed but breaking.

@vrqq did you add the specialisation of cnl::scale or did you replace an existing specialisation?

vrqq commented 1 year ago

After one years I retry to fix this issue, and there still some bug I cannot fixed. (I made a pull request into 991 branch it into branch) The code previous I posted, was break the coding style of template partial specialization. For the third argument, I should keep it fixed on the special type, instead of using template<typename, typename> to match any _impl::number type.

After reading the code in project, I found out the design-rule of struct scale. The third argument class Rep should be specialized in different files, such as _roudinginteger.h

So It may more suitable to move into https://github.com/johnmcfarlane/cnl/blob/991/include/cnl/rounding_integer.h#L74

There's some problem on test like std::hash<path> in cppcon2017.cpp. In the code below, there still have problem on GCC and Clang compiler. https://github.com/johnmcfarlane/cnl/blob/main/test/unit/presentations/cppcon2017.cpp#L23

And conan in pip has been updated into 2.0, but a lot of package hasn't upgraded in there repo.