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/394daf2624f9a001de6cf8c392e42398... (not guaranteed to trigger) [6] https://github.com/cppalliance/decimal/blob/394daf2624f9a001de6cf8c392e42398... [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-characteristi...