Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
vrqq opened this issue Jan 1, 2022 · 6 comments
Open

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

vrqq opened this issue Jan 1, 2022 · 6 comments
Labels

Comments

@vrqq
Copy link

vrqq commented Jan 1, 2022

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)

template<int Digits, class Rep, class RoundingTag>
struct scale<Digits, 2, _impl::number<Rep, RoundingTag>,
_impl::enable_if_t<Digits < 0 && _impl::is_rounding_tag<RoundingTag>::value>>
: _impl::default_scale<Digits, 2, _impl::number<Rep, RoundingTag>> {
};
template<int Digits, int Radix, class Rep, class RoundingTag>
struct scale<Digits, Radix, _impl::number<Rep, RoundingTag>,
_impl::enable_if_t<0 <= Digits && _impl::is_rounding_tag<RoundingTag>::value>> {
CNL_NODISCARD constexpr auto operator()(_impl::number<Rep, RoundingTag> const& s) const
-> decltype(_impl::from_rep<_impl::number<Rep, RoundingTag>>(
scale<Digits, Radix, Rep>{}(_impl::to_rep(s))))
{
return _impl::from_rep<_impl::number<Rep, RoundingTag>>(
scale<Digits, Radix, Rep>{}(_impl::to_rep(s)));
}
};

@vrqq vrqq added the question label Jan 1, 2022
@johnmcfarlane
Copy link
Owner

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
Copy link
Author

vrqq commented Feb 7, 2022

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?

  • Is that a right change?
  • Any other improvement suggestion?

@johnmcfarlane johnmcfarlane reopened this Feb 8, 2022
@johnmcfarlane
Copy link
Owner

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 johnmcfarlane mentioned this issue Feb 8, 2022
@johnmcfarlane
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

vrqq commented Mar 10, 2023

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 rouding_integer.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.

vrqq added a commit to vrqq/cnl-fixing that referenced this issue Mar 10, 2023
johnmcfarlane pushed a commit that referenced this issue Dec 23, 2023
johnmcfarlane pushed a commit that referenced this issue Dec 23, 2023
Co-authored-by: vrqq <vrqq3118@163.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants