Decimal Review Result and Summary.

From: John Maddock (jz.maddock_at_[hidden])
Date: 2025-02-01 18:36:03

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