Re: Decimal: Formal review begins.

From: Matt Borland (matt_at_[hidden])
Date: 2025-01-20 14:29:34

> Hi all,
> 

> This is my review of the proposed Boost.Decimal. First of all, thanks
> to Matt and Chris for proposing the library and answering my
> questions, and to John for managing the review.
> 

> - What is your evaluation of the design?
> 

> Good and clean in general. I like that it follows STL structure as
> much as it can, making it predictable, in general. Some comments:
> 

> 1. Docs state that only the convenience header (boost/decimal.hpp) as
> opposed to individual headers (e.g. boost/decimal/charconv.hpp). I
> think that this might not be the best. It goes against the "don't pay
> for what you don't use" principle (in terms of compile times), and
> doesn't follow the established Boost practice. Including
> boost/decimal.hpp takes around 4.5s, while single headers like
> boost/decimal/decimal32.hpp take just 1.5s. Boost has been criticized
> for its compile times a lot, and I think we shouldn't be indulgent
> with this. Also, tools like clangd tend to follow include-what-you-use
> schemes, so fixing this helps them. My proposals around this issue
> are:
> 1.a. Encourage the use of individual headers in the docs.
> 1.b. Move all public functionality to public headers. For example,
> this means moving io.hpp from boost/decimal/detail to boost/decimal.
> 1.c. Make individual headers standalone. This means that the code for
> suppressing warnings in boost/decimal.hpp should be moved to a
> separate header that is included in all the public headers. See how
> Boost.Asio does it for an example.
> 1.d. Test that individual headers work. For instance, including
> boost/decimal/charconv.hpp errors in my machine. Boost.Beast does
> this, for instance.
> 

> 2. For each decimal size, there's two types: decimal32 is the storage
> optimized one, and decimal32_fast is the operation-optimized one. Like
> Peter, I'm not convinced that making the storage optimized be the
> default one is adequate. I understand that this is what the TR states,
> though. I'd advise to consider Peter's suggestions [1] [2]. I don't
> have enough field experience to know whether this is sufficiently
> relevant, though. In any case, the documentation should state when to
> use the standard ones vs the fast ones.
> 

In this case it is the naming scheme provided by IEEE-754, which I think we would be remiss to ignore. I think the suggestion from Peter (and you below in bullet 3) to add implicit lossless conversions is good. <stdfloat> has far stricter rules than builtin floating point types but allows promotion up implicitly, but errors on shortening.

> 3. I found conversions between decimal types unintuitive. I'd expect
> conversions that don't imply loss of precision to be implicit. For
> instance, I think that these should be implicit:
> 

> * decimal32 to {decimal32_fast, decimal64, decimal64_fast, decimal128,
> decimal128_fast}.
> * decimal64 to {decimal64_fast, decimal128, decimal128_fast}.
> * decimal128 to decimal128_fast.
> * (Same for the fast types).
> 

> At the moment, all decimal <=> decimal conversions are explicit,
> 

> regardless of whether they imply loss of precision or not. I'd
> consider re-thinking this.
> 

> 4. I was surprised to find support for an inherently insecure function
> like sprintf. sprintf(char* buffer, const char* format, T... values)
> writes characters to buffer but doesn't accept a buffer length, making
> it prone to buffer overflow attacks. The current implementation looks
> to be incorrect, as it calls snprintf(buffer, sizeof(buffer), ...),
> where buffer is a pointer, effectively assuming the buffer size is
> always sizeof(void*). No tests cover this and no-one has reported
> this, making me think that no-one might be using it. I'd advise
> removing sprintf.
> 

Dropping sprintf should be fine in 2025. I concur that I don't think anyones using it. 

> 5. I'm also not convinced about the value added by the other cstdio
> functions, namely printf, fprintf and snprintf. They only support
> decimal arguments. This means that printf("Value for id=%s is %H", id,
> value) is an error if id is a const char*. Integers work because they
> are cast to decimal types, which I found surprising, too. Given an int
> i, printf("Value for %x is %H", i, value) works but doesn't apply the
> hex specifier. The integer is promoted to a decimal type, which is
> then printed. I'd have expected the latter to be a compile-time error.
> I'd say that this will be the case if you replace std::common_type by
> std::conjunction as Peter suggested.
> 

> If you're not going to support all standard type specifiers, I'd think
> of exposing a std::format-like function that works in C++14. Something
> like decimal::format_decimal and decimal::print_decimal. This is just
> a suggestion, though. I understand that, if this is pushed to the
> standard, having these functions might be valuable.
> 

The implementations of those functions was more in the realm of reference for how you would add the functionality to the existing STL should they be standardized. Making a complete working reimplementation of all those clib functions would be a huge undertaking for little benefit. 

Since <charconv> is available at C++14 doesn't that generally cover what format_decimal, and print_decimal would do? I specifically avoided using the STL here because requiring C++17 for a two member struct and an four member enum seemed silly. 

> 6. Literals should be in a separate namespace, namely
> boost::decimal::literals or similar, and not in the boost::decimal
> namespace.
> 

Agreed and there's an open issue for that

> 7. Are concept names supposed to be public? If they are, please
> document them as regular entities. If they are not, please move them
> into the detail namespace.
> 

> 8. The interface exposes functions taking and returning detail::int128
> objects. I don't think this should happen. If a type appears in a
> function signature, it should be public. The documentation actually
> lists uint128 with its members, so this makes me think that the type
> is in fact public, and should be placed in the boost::decimal
> namespace.
> 

That's fair.

> 9. Is boost::decimal::to_string public? It's handy, and in the public
> namespace, but in a detail/ header and not documented. I'd advise to
> make it public by moving the header to boost/decimal/ and documenting
> it.
> 

I'll move it. I think it goes along with your printing point, and is generally something I would expect to be provided for a numeric type. 

> - What is your evaluation of the implementation?
> 

> I'm not knowledgeable of decimal floating point math as to evaluate
> the mathematical correctness of the implementation. My comments here
> address general C++ aspects of the implementation.
> 

> In general, I've been able to follow it easily. It's well written.
> There are some points that should likely be addressed before the
> library goes into Boost, though:
> 

> * The significand_type, exponent_type, and biased_exponent_type
> typedefs in the decimal types are public but undocumented. If they're
> private, they should be placed in private scope. Otherwise, they
> should be documented.
> * Code shipped to the user (i.e. anything in include/) should not
> contain functions or definitions that are exclusive to the tests. The
> following should be moved:
> * BOOST_DECIMAL_REDUCE_TEST_DEPTH (include/boost/decimal/detail/config.hpp)
> * debug_pattern (include/boost/decimal32.hpp)
> * bit_string (include/boost/decimal128.hpp)
> * operator<< for detail::uint128_t and detail::uint128
> * The macro BOOST_DECIMAL_DISABLE_IOSTREAM seems dubious. Most of the
> bits it disables seem to be either test-exclusive functions (which
> shouldn't be there) or leftover includes (as there is no other
> matching #ifdef in the file). I think it can be removed with a bit of
> refactoring. On the other hand, BOOST_DECIMAL_DISABLE_CLIB looks good
> to me.
> * sprintf and fprintf seem to have no tests.
> * BOOST_DECIMAL_ALLOW_IMPLICIT_CONVERSIONS affects decimal32,
> decimal64 and decimal128, but is only tested for decimal64. Also, it
> doesn't seem to affect the fast types. Is there a reason for it?
> * I'd advise to avoid includes in config.hpp. They get propagated
> everywhere, violating the include-what-you-use principle. I've also
> found them problematic in my modularization efforts in Boost.Charconv.
> * The build.jam file lists the library name as "boost_core" instead of
> "boost_decimal".
> * I'd advise to define BOOST_DECIMAL_CONSTEXPR in config.hpp, rather
> than in a file containing definitions. This is a pattern that has also
> caused me trouble in charconv. Also, BOOST_DECIMAL_CONSTEXPR may be a
> confusing name, since its semantics don't match with BOOST_CONSTEXPR
> semantics in Boost.Config.
> * BOOST_DECIMAL_FAST_MATH seems to heavily affect many of the
> mathematical functions, but there is only a single test for it. Are we
> sure that's enough coverage? Would it make sense to have a full CI
> build defining BOOST_DECIMAL_FAST_MATH?
> * I can see some fuzz testing, which is great. It would be great to
> extend it to cover other character formatting functions like to_chars
> or snprintf.
> * LCOV_EXCL_START/LCOV_EXCL_STOP should only be used in places where
> code flow is unreachable. These places should use
> BOOST_DECIMAL_UNREACHABLE. Other usages distort the code coverage
> metric. Concretely, [4] is related to [5], which is marked as excluded
> from coverage but is reachable.
> * The charconv functions issue some spurious warnings under gcc [7].
> * I found some problems with numeric_limits and subnormal value
> formatting [4] which had already been reported and are in the process
> of being addressed.
> 

> As per Matt's request, I've also included some comments about the
> C++20 module support that this library includes:
> 

> * If you want to follow the conventions that we're using in the
> initiative I've been writing, the module name should be
> "boost.decimal", instead of "boost2.decimal". The module interface
> file should be named boost_decimal.cppm, instead of decimal.cxx.
> * The UINT64_C/UINT32_C macro definitions in config.hpp look
> unnecessary, and may cause trouble in some platforms/configurations
> [3]. You shouldn't need to define these. You get access to these by
> including cstdint in the global module fragment, which you are doing.

I didn't think they were needed, but I have run into issues where the compiler threw a million errors on macros undefined. 

> * The export namespace std block in the interface unit should be
> removed. I don't know for sure, but it's likely undefined behavior in
> theory. Specializations don't need to be exported (although MSVC has
> several bugs here). You're including the entire library in the
> purview, so I don't think these bugs affect you.
> * You shouldn't need to export the forward declarations, either.

I'll remove them and see how it goes, at one point it was needed.

> * Have you tried to run quick_test.cpp with import std? I don't think
> import std is as easy as checking a config macro - you need to enable
> it at the build system level.
> 

> 

> - What is your evaluation of the documentation?
> 

> It's a bit terse. The information you need is mainly there, but it
> could be made better for the newcomer.
> 

> Concretely, a longer exposition explaining the available types and
> operations would have helped me when I started. I feel it jumps into
> the reference section too soon. I'd say it also assumes that the
> reader has read the decimal TR and the IEEE754 spec, which may not be
> the case. I think making the docs more self-contained would be
> beneficial. A section on when to use decimalXY vs. decimalXY_fast
> would be very useful, too.
> 

> A comparison section to other libraries would also be great. Intel
> seems to have a similar library (although it's C, and not sure about
> its license). I've also found libmpdec [8], which powers Python's
> Decimal [9]. I think its scope is completely different to
> Boost.Decimal, though (fixed-point arithmetic only and arbitrary
> precision).
> 

That's fair since I already include benchmarks against a different library.

> A section on "what if I just need fixed-point arithmetic?" would also
> be great. From my own experience and the comments I've seen in this
> review, this seems to be a common use case. I understood that the use
> case is supported, as long as the longest precision you need is <= the
> max precision your decimal type supports (e.g. 7 digits for
> decimal32). A small section stating this would suffice.
> 

> The reference lists all the functions with proper links to the
> equivalent standard library functions, which is helpful. However, I'd
> prefer these links to be just informative, with the reference
> containing a proper description, preconditions, exception
> specification, and so on. As an example, decimal::to_chars with
> chars_format::fixed changed behavior with one of the fixes that was
> merged during the review. 9.1_df would be formatted as "9.1000000"
> before the fix, and as "9.1" after the fix. Both things made sense to
> me. It'd been great if the reference explained this, for instance.
> 

> The documentation is single page, which I always find hard to navigate.
> 

> Some minor points:
> 

> * All code snippets have extra indentation in their first line, making
> them look strange.
> * The "Note" and "Important" admonitions look incorrectly formatted.
> It looks like these should have contained icons, and a fallback piece
> of text is being displayed.

I messaged the guys who maintain the stylesheet.

> * There's a typo in basics.adoc ("souce" instead of "source") making
> the second code snippet in the docs to not be syntax highlighted.
> * Concepts are not listed in the reference.
> * In the synopsis of decimal32 and siblings, members are not indented.
> * It would be nice to make clearer that BID is "fast but more space"
> and DPD is "slower but compact" here:
> https://cppalliance.org/decimal/decimal.html#conversions
> * The numeric_limits synopsis includes an #ifdef _MSC_VER that is
> really an implementation detail. I don't think that MSVC declaring
> numeric_limits as a class is relevant to the user anyhow.
> * It would be great if to_chars listed the possible errors under which
> the function fails, and the error codes it produces.
> * In the documentation, numbers like e_v are in the boost::decimal
> namespace, which doesn't match the actual code
> (boost::decimal::numbers namespace). I think the latter is correct.
> * In the documentation, numbers are defined as static constexpr, not
> matching the code. They should be listed as "inline constexpr".
> * The scroll bar is located in the center of the screen, rather than
> on the right side.
> 

> - What is your evaluation of the potential usefulness
> of the library? Do you already use it in industry?
> 

> I think it's useful. According to the author, it's already in use in
> some financial companies, which is great. SQL databases also have
> DECIMAL types. I think that having it in Boost adds value.
> 

> - Did you try to use the library? With which compiler(s)? Did
> you have any problems?
> 

> I've used it to implement support for the DECIMAL type in Boost.MySQL.
> MySQL's DECIMAL(p, s) type [10] is a fixed-point decimal with a
> configurable precision p. p ranges from 1 to 65 digits. s is the
> decimal scale, with 0 <= s <= p. That is, DECIMAL(5, 2) stores values
> like 134.15. I've added support for using decimal32, decimal64 and
> decimal128 (and their fast counterparts) in the following contexts:
> 

> * In the static interface, so you can define row types like "struct
> employee { decimal::decimal32 salary; };". MySQL sends the type's
> precision before the rows, so I check it before incurring in rounding
> errors (using a decimal32 with a DECIMAL(10) is always an error).
> * In SQL formatting, so you can issue queries containing decimals like this:
> 

> conn.execute(mysql::with_params("SELECT * FROM employee WHERE salary >
> 

> {}"), result);
> 

> PR is here: https://github.com/boostorg/mysql/pull/399
> 

> This exercises the charconv API and some utility functions. I've
> tested it in my CIs, covering gcc >=5, clang >= 3.6, MSVC >= 14.1.
> 

> Aside from some problems in older compilers (not listed as supported
> in the docs), things have worked fine. I've encountered a couple of
> bugs that I reported and have been promptly fixed.
> 

> - How much effort did you put into your evaluation?
> A glance? A quick reading? In-depth study?
> 

> I've spent most of this week reading the documentation, the DTR and
> the public includes, building the MySQL prototype, asking questions,
> reporting potential problems and writing this review. I've spent
> around 30h in the process.
> 

> - Are you knowledgeable about the problem domain?
> 

> I'm no expert in decimal number arithmetic - just a user. I know
> nothing about their internals.
> 

> - Affiliation disclosure
> 

> I'm currently affiliated with the C++ Alliance, as is Matt Borland.
> 

> Ensure to explicitly include with your review: ACCEPT, REJECT, or
> CONDITIONAL ACCEPT (with acceptance conditions).
> 

> My recommendation is to CONDITIONALLY ACCEPT the proposed library into
> Boost, under the following conditions:
> 

> * Individual include files (e.g. boost/decimal/charconv.hpp) are
> supported, tested and its usage encouraged.
> * sprintf should be fixed or, better, removed.
> 

> I recommend applying the rest of the feedback, but that's up to the authors.
> 

> My general feeling about the library is good. A lot of effort from the
> authors has gone into this. The library covers real use cases and is
> used as of today. I think Boost is better with this library than
> without it.
> 

> Many thanks Matt, Chris and John for your work.
> 

> Regards,
> Ruben.
> 

Thank you for your detailed review. Your conditions are fair and I'll add issues and tag you in them.

> 

> [1] https://lists.boost.org/Archives/boost/2025/01/259012.php
> [2] https://lists.boost.org/Archives/boost/2025/01/259021.php
> [3] https://stackoverflow.com/questions/52490211/purpose-of-using-uint64-c
> [4] https://github.com/cppalliance/decimal/issues/794
> [5] https://github.com/cppalliance/decimal/blob/394daf2624f9a001de6cf8c392e423983d0d60d4/include/boost/decimal/charconv.hpp#L341
> (not guaranteed to trigger)
> [6] https://github.com/cppalliance/decimal/blob/394daf2624f9a001de6cf8c392e423983d0d60d4/include/boost/decimal/charconv.hpp#L220
> [7] https://github.com/cppalliance/decimal/issues/801
> [8] https://www.bytereef.org/mpdecimal/doc/libmpdec/index.html
> [9] https://docs.python.org/3/library/decimal.html
> [10] https://dev.mysql.com/doc/refman/8.4/en/precision-math-decimal-characteristics.html