Re: Decimal: Formal review begins.

From: Ruben Perez (rubenperez038_at_[hidden])
Date: 2025-01-19 19:03:21

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.

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.

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.

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

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.

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.

- 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.
* 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.
* 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).

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

[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