On Thu, Jan 23, 2025 at 4:18 PM Jeff Garland via Boost < boost@lists.boost.org> wrote:

I didn't have time to play with it unfortunately -- godbolt would be helpful.

This is a bit late now since you wrote your review, and it is last day,

but might help somebody finishing their review now. Library is header only. So no need to integrate it into modular boost checkout or something. Just DL the library and use it. Adding it to cmake file for me was 2 changed lines.

 > trust the authors to improve> documentation, api, and> performance as a matter> of continued development.
Thank you, Jeff, for yourinsightful, informative andprogress-oriented review andcomments.
Yes, the plan is for evolutionof Decimal and improvement,partially along the lines ofall the great comments we havegarnered from your and otherreviews.
> Let me say a few things> about some discussions in> the review threads up-front.
 ... Jeff's comments...
We appreciate your commentsand have already begun furtheronline/offline chats and firedup a few coding issues regardingsame.
Thanks again.
- Chris


    On Thursday, January 23, 2025 at 04:18:22 PM GMT+1, Jeff Garland via Boost  wrote:  
 
 Thanks to Matt Borland and Chris Kormanyos for this good work. I vote to
ACCEPT the library.  I trust the authors to improve documentation, api, and
performance as a matter of continued development.  Let me say a few things
about some discussions in the review threads up-front.

*Standardization:*

I'm an advocate for it and would be willing to help the authors with the
process.  Rationale: 1) this standardizes existing practice (aka IEEE 754),
something like it is supported in many languages (specifically C23), and
there's significant use cases for this in many domains even if it's
slightly slower.  2) While a standalone library can go far, often a
compiler/library can more easily optimize fundamental things like numeric
types. 3) I spoke with Dietmar about the review and standardization -- he's
still an advocate for having this capability,  but has higher priorities at
the moment.

*N2849*:

It's a decent design for 2009 -- less so in 2025.  Specifically, w.r.t.
conversion we should only perform automatic conversions from types (see
below).  The library already takes care of from_chars, format and some
other more modern things N2849 wouldn't address.

*C23 and _Decimal**

Should this library consider being a layer over C23 types where that's
possible?  GCC supports them at least in a limited fashion.

https://gcc.gnu.org/onlinedocs/gcc/Decimal-Float.html


> Does this library bring real benefit to C++ developers for real world
use-case?
> Do you have an application for this library?

Yes and yes.

> Does the API match with current best practices?

I have some quibbles here.

*Construction*:

template constexpr decimal32(T coeff, T2 exp,
bool sign = false) noexcept;

Is the sign flag helpful here?  It seems simpler to just handle a negative
coefficient -- like it already does.  When I look at it there's already
cognitive dissidence in my brain about if the sign flag *overrides* or
*inverts* the coefficient sign.  The N2849 api doesn't have this flag.


*Fast Types:*

I read through the list conversation about the _fast types. My suggestion
is to drop them.  Simplifies everything for users and implementers. The
movement in c++ over the last decade(s) has been to precisely say the size
of the type to ensure correctness and not 'let the platform decide' (aka
uint32_t, int64_t, etc).  In my comment on documentation you'll notice the
c++20 extended floating point types *only provide* fixed sizes. Also N2849
and C23 don't support _fast types for decimal.


*Conversions:*

I'd like to see the library *pull back* on convertibility.

long double ld{1.23456789234567};
decimal32 d(ld); // what happens -- round, truncate, doesn't compile?

The best answer is doesn't compile. There's been significant work in the
standard now to define floating point rank and limit conversions with the
extended floating point types. Specifically:

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p1467r9.html#rank

There's also other work on type traits and value preserving conversions in
the SIMD capabilities. Also, std::chrono basically already does this -- any
value destroying conversion has to be explicit.  At a minimum the answer
needs to be better documented - aka does what 754-2008 says?

For other examples of things I'd remove and make a free function cast/round
-- aka explicit user decision:

class decimal64 {
  ...
  explicit constexpr operator std::int8_t() const noexcept;

This is highly likely to not be value preserving so shouldn't be possible
without a decision on how to handle overflow.

class decimal64 {
  ...
  explicit constexpr operator decimal32() const noexcept;

*Literals*

constexpr auto operator  "" _DF(const char* str) -> decimal32constexpr
auto operator  "" _df(const char* str) -> decimal32

Why not _d32, _d64, _d128? _df makes me think fast, not 32.

> Is the documentation helpful and clear?

Not bad, but as usual improvements can be made.  As a user I'm going to
need to select one of these types -- I'd love to see a table like this one
at cppreference for named floating point types:

*Type name*
Defined in header
  Literal
suffix Predefined macro C language type Type properties
bits of storage bits of precision bits of exponent max exponent
std::float16_t f16 or F16 __STDCPP_FLOAT16_T__ _Float16 16 11 5 15
std::float32_t f32 or F32 __STDCPP_FLOAT32_T__ _Float32 32 24 8 127
std::float64_t f64 or F64 __STDCPP_FLOAT64_T__ _Float64 64 53 11 1023
std::float128_t f128 or F128 __STDCPP_FLOAT128_T__ _Float128 128 113 15 16383

std::bfloat16_t bf16 or BF16 __STDCPP_BFLOAT16_T__ (N/A) 16 8 8 127

Not sure how it renders in email so source:
https://en.cppreference.com/w/cpp/types/floating-point

Not looking for exact replication, but something like this.

In the *using the library* section I'd like to see some:
- examples of using it it math
- examples of the behavior of NAN/inf values
- conversions from other types like float expanded

Where is the design decisions section?  Some of the things I'm asking about
should be covered in the docs.

> Did you try to use it? What problems or surprises did you encounter?

I didn't have time to play with it unfortunately -- godbolt would be
helpful.

That's all for now.

Jeff

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost