Boost

boost@lists.boost.org

February 2025

15 participants
11 discussions

Decimal Review Result and Summary.
by John Maddock 02 Feb '25

02 Feb '25

First off I'd like to thank Matt and Chris for submitting the library for review, and for all the reviewers who submitted comments. My apologies for not posting this sooner, but this has been a difficult review to reach a definite result on. First the headline result: the result of the review is INDETERMINATE. What this means in practice: I'd like to encourage Matt and Chris to come back for a mini-review at a later date which should focus exclusively on the open issues listed below. I forget which library we applied this to in the past, but I believe the result is not without precedent. I note that a greater weight of reviews, and/or a clearer path forward may well have tipped the balance here. And of course as someone who's worked with Matt and Chris, I may well be being over-harsh in order to avoid bias... or perhaps I'm just over thinking the whole thing... In my notes below, I've split my comments into: * "Settled Issues", these were issues that were raised at some point but found not to be true issues. * "Todo", straightforward usability and implementation issues such as we get from any review process. I've filed matching GitHub issues for each of these. * "Open", these are the harder knotty problems, where it's not clear (yet) what the solution is. RATIONALE: we had five reviews (4 public, one direct to me), of which 4 were generally positive, and one was for reject. In addition, some of the positive reviews were directly contradictory in the direction they thought the library should progress in ("we should only support the fast types", vs "we should only support the fixed width types" etc). There were also some conditions for acceptance - mostly performance based - where it's not clear whether the objectives are possible in a sane time-frame. For example the often referenced Intel C library is frankly huge (source files > 6Mb in size) and makes heavy use of the "every function is a macro" C idiom, which at least for me renders the code virtually unreadable. As things stand I consider it unreasonable to ask the authors to replicate that work. In late breaking news, I also hear from Matt that GCC and IBM both "cheat" by using (mutually incompatible) non-IEEE binary encodings. There is also the "do we actually need decimal arithmetic" question - it is clear that many people think we do - there have been repeated attempts from researchers at IBM, Intel and others to push this towards standardization, in addition to some (rather patchy) hardware support. It's in C23, which means eventually C++ compiler support will be required too. Oh and python has a decimal library. However the authors need to articulate the case better - the killer application seems to be exact round tripping from human-readable and machine representations (ie exact representation of human data input). John Maddock (Decimal Library Review Manager) OPEN ISSUES ~~~~~~~~~~~ The case for decimal floating point needs to be better made - despite support from IBM and Intel, plus repeated standardization efforts, true killer examples seem to be thin on the ground. Perhaps liaison with other experts in the field would be helpful here (I note that Robert Klarer at IBM and also once around these parts has been involved in decimal proposals, https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2006/n1977.html)? Should arithmetic operations always be performed on the performance optimized forms? If so, perhaps storage optimized forms should have no operators (or their operators return the unpacked versions)? Review managers note: can we use DEC_EVAL_METHOD to set behavior here? How do we address the performance gap? Add the option to wrap the Intel library? Rewrite using the same methodology as Intel (this looks like a truly massive amount of work)? Several reviewers make this a condition of acceptance however. Should we use _fast integers or width specific ones? The answer appears to be platform specific. Perhaps it's a question of changing the correct instances of _fast integers rather than all of them? Should we abolish the _fast types and only provide fixed-width std compliant types: this is the forward looking option in line with what the standards are doing... but it directly contradicts the requests of some other reviewers. SETTLED ISSUES ~~~~~~~~~~~~~~ The decimal32 decimal64 names etc are reserved by various standards especially IEEE-754, we need to stick to those names. Over long literals are acceptable without warning or error, though a static warning facility would be nice. One reviewer suggests removing BOOST_DECIMAL_FAST_MATH, however as it performs similar optimizations to other libraries / compiler options, it looks reasonable to stay if the authors wish. One reviewer suggested shortening the namespace names - this is a tricky one, I see the issue, but we have a tradition (rightly or wrongly) for long_descriptive_names in boost. Plus there are always using declarations and/or namespace aliases. I will leave this to the authors to decide. long/int overloads of scalbln are fine by me as these are std mandated, however the reviewer makes a valid point that these are useful only on 16 bit platforms (I do note that the library is intended to be micro-controller compatible though, so this may well be an issue there). One reviewer suggests passing arguments by value is a mistake especially for 128 bit types, other reviewers refuted this. Other than some experimentation / double checking on windows and non-Intel platforms this does not appear to be an issue in practice. Suggest no change unless the double checks prove otherwise, a comment in the source or the design rationale may well be welcome. One reviewer suggested using CLZ in num_digits, but this has been checked and is slower (a comment in the source to that effect would be welcome. Note that std::lower_bound is not constexpr until C++26. REQUESTED CHANGES ~~~~~~~~~~~~~~~~~ Literals should be in their own namespace. https://github.com/cppalliance/decimal/issues/827 Allow implicit conversions for all conversions which do not lose precision: https://github.com/cppalliance/decimal/issues/828 Remove sprint support. https://github.com/cppalliance/decimal/issues/829 Split into "use what you need" smaller headers, in particular iostream and std lib support code should be in separate optional headers rather than in #ifdef blocks. https://github.com/cppalliance/decimal/issues/830 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. https://github.com/cppalliance/decimal/issues/831 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. https://github.com/cppalliance/decimal/issues/832 Collected documentation improvements: https://github.com/cppalliance/decimal/issues/833 Do the exponent_type typedefs need to be public? Do they need to be 64 bit? I suspect the latter is all about extracting the bits from the representation, and the former is a convenience to avoid a long list of friends - although the list doesn't look *that* long? If they remain public then a comment in the source with rationale would suffice. https://github.com/cppalliance/decimal/issues/834 Can we add (possibly explicit) constexpr construction from string literal? https://github.com/cppalliance/decimal/issues/835 There is an undocumented public constructor: constexpr decimal32(bool coeff, T exp, bool sign = false) noexcept; which appears to be a mistaken addition? https://github.com/cppalliance/decimal/issues/836 I wonder if constexpr decimal32(T coeff, T2 exp, bool sign = false) noexcept; is under-constrained? The sign argument appears to be superfluous given that coeff can be negative, and we know up front the size of integer required to represent the coefficient and exponent so we might as well make this a non-template: smaller integer arguments will get promoted and larger arguments will quite correctly issue a warning (I wouldn't use a type smaller than int though, otherwise the things get altogether too picky): https://github.com/cppalliance/decimal/issues/837 There are a few places where pow10 is used with a constexpr argument, but while the function is constexpr it is not used in a constexpr context. The example picked up from the review was auto res_sig {(static_cast<unsigned_int128_type>(lhs_sig) * static_cast<unsigned_int128_type>(rhs_sig)) / pow10(static_cast<unsigned_int128_type>(13))}; in mul_impl.hpp but there may be others. https://github.com/cppalliance/decimal/issues/838 Consider using delegating constructors in for example constexpr decimal128::decimal128(bool coeff, T exp, bool sign) (if we keep this... but check for other opportunities). https://github.com/cppalliance/decimal/issues/839 Ensure that +0 and -0 are equivalent. https://github.com/cppalliance/decimal/issues/840 Make uint128 a non-detail type given that it is used in public interfaces. Such a shame that it can't be a real std::uint128_t, but perhaps a conversion operator could be provided when the type is available? https://github.com/cppalliance/decimal/issues/841 Rationalize traits class usage to make the code easier to read: https://github.com/cppalliance/decimal/issues/842 Code duplication: comparison operators calling into `equal_parts_impl()` for `decimal32_fast`, but the definition being placed directly into the comparison operator functions for `decimal64_fast` and decimal128_fast`. For me, this makes it seem like these 3 types must be implementing the comparison operators in different ways, when in reality they are doing exactly the same calculation, implemented in 3 separate places. https://github.com/cppalliance/decimal/issues/843 Code duplication with `bias_v`: https://github.com/cppalliance/decimal/issues/844 Explicit conversions need much better documentation for overflow behavior - I had to look at the source for to_integral to figure this out - I'm also wondering if returning zero on error is the right solution or if the result should be saturated? https://github.com/cppalliance/decimal/issues/845 Remove the footgun of lossy conversion from decimal floating point types. https://github.com/cppalliance/decimal/issues/846

6 9