Re: Decimal: Formal review begins.

From: Peter Turcan (peterturcan_at_[hidden])
Date: 2025-01-21 05:53:42

John

Here is my review of the Decimal library documentation. I am the Technical
Writer for the CppAlliance.

I appreciate the effort that has gone in to try to make this doc set
complete.

Main issues for me are the absence of a table of contents for the docs
(though perhaps a build script will create this?), moving the finance
examples to a much higher profile position within the docs - as this seems
to be the main use case, perhaps articulating other compelling (or author
favorite) use cases, and sentences explaining what the given
examples/functions/macros do.

Hope the following review helps!

- best
Peter

*Decimal library documentation review*

Great that the *Use Cases* is present as part of the introductory topics.
What about other scenarios than finance - perhaps statistical applications
(variance, standard deviation, correlations), Astronomy (planetary orbits
over long time frames, gravitation), Mathematical Proofs (verifying
conjectures, symbolic algebra - where exactness is key), Cryptography,
Machine Learning (optimization algorithms, autonomous driving!), Graphics
(to avoid artifacts)?  If you have any favorites - mention, ideally with an
example or two.

Perhaps this would be created by a build script, but the docs could really
use a *Table of Contents*, with all the headers and subheaders clear to
see. This also helps us understand the structure and scope of the library.

Odd formatting of the "Note" with Note centered vertically in the box,
should probably be top left. Same throughout the docs - for "Important"
topics as well as Note.

Some odd leading spaces in the examples and source code "   #include"
rather than "#include", for example.

"A short example of the basic usage:" - would be better to explain what the
example shows, even if it does seem obvious - make it clear for the
first-time user of this type of library.

As finance has been explicitly named as a compelling use-case, perhaps add
a meaningful (commented) finance example to the "Basic Usage" section. Or,
move the Financial Applications section up here - or both.

*API Reference*

Add an intro sentence as to what is available in the reference. Such as
"this section contains complete details on all the types, structures,
enums, constants and macros that are defined in this library" - make sure
that this sentence is true - have you added complete details on all this
stuff - no matter how peripheral they may be.

Good to see links to the API components.

Not sure why there is */* see 3.2.8 */*  in that section, yet "3.2.9" does
not. Perhaps remove the "/* see 3.2.8 */ comment - the introductory comment
should do it. However, ensure there are NO exceptions to a general
statement like this. If there are, add a comment to the individual entry to
illuminate.

Perhaps add a bit more information to the *Formatted input:* section - OK
these are "locale dependent" but what do they do? I assume take an input
stream of characters - but consider explaining this and what terminates the
input stream? And the same comment for the output.

The *Description *of *Decimal32 *is given as a list of bullet points,
whereas the information is more suited to a table. The first column would
be something like "Attribute" the second "Value", such as:

*Attribute        Value*
Storage width        32 bits
Precision                16 decimal digits
Max exponent

- the point is:  bullet points are overused and the content suggests a
table. Tables look nicer than bullet lists.

Same comment on bullets versus tables for all the other entries in the API
reference.

For all the functions, consider listing the Errors and Exceptions that
might be thrown - if any. For maximum value, consider adding what to do
about it if a particular error is thrown.

For functions, add an introductory sentence under the name of the function
stating what it does - even if it seems obvious. If there are any
limitations or gotchas this is a good place to articulate them.

*cfloat support *- syntax style and coloring appears to have been lost.

Macros - does describe what is disabled or defined, but consider adding a
sentence as to why you would do this. "Disabling the use of I/O streaming
enables........what?" Same for all the other macros. Add the use-case or
benefit.

*Examples*
Add a sentence, or two, as to what the example shows. Great if limitations
and gotchas are noted here too. For example "Generic Programming" - great
there is example code but what does the example show?

*Financial Applications*
Great - but consider moving much closer to the top of the documentation.
Example use cases are some of the first things a dev wants to see.

*Design Decisions*
Perhaps move this section ABOVE the API Reference. It is short and
explanatory as to the thinking behind the library, and is important to
understand early on.

References and Copyright etc. are OK at the end of the doc.

On Mon, Jan 20, 2025 at 12:56 PM Matt Borland via Boost <
boost_at_[hidden]> wrote:

>
>
>
>
>
> On Monday, January 20th, 2025 at 2:29 PM, Fernando Pelliccioni via Boost <
> boost_at_[hidden]> wrote:
>
> >
>
> >
>
> > On Wed, Jan 15, 2025 at 10:34 AM John Maddock via Boost <
> > boost_at_[hidden]> wrote:
> >
>
> > > The review of the proposed Decimal Number library by Matt Borland and
> > > Chris Kormanyos begins today and runs until 22nd Jan.
> > >
>
> > > You will find documentation here:
> > > https://cppalliance.org/decimal/decimal.html
> > >
>
> > > And the code repository is here:
> https://github.com/cppalliance/decimal/
> >
>
> > Hi everyone,
> >
>
> > First of all, a big thank you to Matt and Chris for your hard work, I
> > personally find this library very useful.
> > Here are a few quick comments:
> >
>
> > 1. Literals
> > I agree with earlier suggestions about placing them in a separate
> > namespace, such as boost::decimal::literals, rather than boost::decimal.
> > This matches typical Boost conventions.
> >
>
> > 2. Headers
> > I also concur with the feedback regarding compile times. Relying
> > exclusively on <boost/decimal.hpp> can be slow. Please provide smaller
> >
>
> > headers.
> >
>
>
> There are issues open for 1 and 2 to be addressed.
>
> > 3. Fast vs. Non-Fast Variants
> > I think the authors’ choice of default is probably fine, though I’m not
> > 100% certain. In any case, clearer documentation on when to pick the
> > standard type vs. the fast variant would be really helpful. Some guidance
> > on weighing storage vs. speed would benefit end users.
> >
>
> > 4. <cstdio> Support
> >
>
> > I share the concerns about potentially unsafe functions and limited
> > specifier support. Maybe focusing on <format> (for C++20) or a safer,
> >
>
> > custom formatter would be more straightforward and less error-prone.
> >
>
>
> I think pushing people to charconv is the better move. <format> support
> needs GCC >= 13, Clang >= 18, or _MSC_VER >= 1940. That's a high bar for a
> C++14 library.
>
> > 5. Concepts
> > If the Concepts defined here are intended for user code, I agree they
> > should be public and documented. I would like to use them in my C++20
> (and
> > beyond) code.
> >
>
> > 6. Hardware Decimal Support
> > I don't see explicit mention of how this library could leverage hardware
> > decimal capabilities, but I imagine that could be added in the future,
> > possibly via a pull request from someone who has access to that hardware.
> > It is a nice to have feature but not a must have. I would not stop the
> > library from being released without it.
> >
>
> > 7. Compiler-Explorer support
> > It would be great if this library was supported on Compiler-Explorer for
> > doing some testing. I know it is not related to the library but it helps
> a
> > lot for reviewing it.
> >
>
> > 8. Benchmarks
> > I plan to run some additional benchmarks on my own machines.
> > One thing I'm curious about is how the built-in GCC _Decimal32/_Decimal64
> > types compare. Do they behave more like the "standard" or the fast
> variants
> > in practice?
> >
>
>
> "Standard", but I will note the behavior of the decimalXX, and
> decimalXX_fast are identical except the latter does not support
> sub-normals.
>
>
> > Thanks again! I think this library is shaping up wonderfully and can't
> wait
> > to see how it evolves.
> >
>
> > Best,
> > Fernando
> >
>
> > _______________________________________________
> > Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>