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