Re: Decimal: Formal review begins.

From: Matt Borland (matt_at_[hidden])
Date: 2025-01-16 15:05:33

> 

> Thanks. I'm trying to understand whether sticking to the TR has enough
> value. If it doesn't, maybe we could consider applying Peter's
> comments about making decimal32 be the fast one, or dropping sprintf.
> 

The TR was a good starting point, but yes we have added much since then to make the types as seamlessly interoperable with the STL as we can. Like two weeks ago someone asked for <format> support and we provide that which obviously won't be in a TR from 2009. Is there any real gain from dropping a standard function that's already generally implemented? I don't think so especially if we pursue standardization. It's not a totally complete implementation of sprintf but it would groundwork for bolting on support to existing std:: implementations. 

> Some more questions:
> 1. As a user, when should I pick decimal64 vs decimal64_fast? I intend
> to implement support for your decimal types in the static interface of
> Boost.MySQL as part of this review - should I support decimalXY,
> decimalXY_fast, or both?

It would be a pretty easy template to support decimalXY and decimalXY_fast. I know for fact decimalXY is being used out in industry, but not sure about decimalXY_fast. It would be more space efficient to store as decimalXY.

> 2. Is there a rationale behind only supporting the convenience header,
> as stated in the docs? Including the entire library in my machine
> (gcc-12, Ubuntu 22.04, -std=c++23) is about 4.5seconds - similar to
> the entire Asio in magnitude. Including only decimal32.hpp cuts the
> time to around 1s.

I never tried many of the permutations of headers outside of the convenience one. The library is structured to match the STL so that it is unsurprising to the average user. I think you could pick and choose if you wanted to.

> 3. In the line of the previous question, is there a reason to have
> BOOST_DECIMAL_DISABLE_IOSTREAM instead of splitting iostream
> functionality to a separate header? In my experience, the more config
> macros you have, the more chances of getting bugs. Also, is the test
> suite being run with these macros defined?

We have a the options to disable a bunch of the clib functionality so that the library can run on embedded platforms. We do have QEMU of an STM board in the CI which tests all of this. Why test embedded you ask? It's not uncommon for finance devs to run on bare metal platforms. 

> 4. From sprintf's documentation: "In the interest of safety sprintf
> simply calls snprintf with buf_size equal to sizeof(buffer). ". This
> doesn't look right. This is what the implementation looks like:
> 

> template <typename... T>
> 

> inline auto sprintf(char* buffer, const char* format, T... values) noexcept
> #ifndef BOOST_DECIMAL_HAS_CONCEPTS
> -> std::enable_if_t<detail::is_decimal_floating_point_v<std::common_type_t<T...>>,
> 

> int>
> 

> #else
> -> int requires
> 

> detail::is_decimal_floating_point_v<std::common_type_t<T...>>
> 

> #endif
> {
> return detail::snprintf_impl(buffer, sizeof(buffer), format, values...);
> }
> 

> If I'm reading this correctly, this is calling snprintf_impl with the
> sizeof a pointer, which is probably not what we want. You'd need to
> template the function on the buffer size and take a char array
> reference to make this secure.
> 

I'll take a look.

Matt