Re: My review of Decimal

From: Matt Borland (matt_at_[hidden])
Date: 2025-01-21 13:56:33

> Hi everybody,
> 

> PDF size exceeds ML limit so here is google drive link
> https://drive.google.com/file/d/1kH9WkeTDxN7FFqM_g7hV_-e3kJPlTGBr/view
> 

Since this is a PDF I'll just pick out an add headers to the parts I'll address

C++14 - There's a bifurcation here between people who want to Boost to support ancient standards and toolchains forever, and those who want cutting edge. In the implementation side C++14 has been a happy medium where I don't see complaints anymore about supporting older standards, and it's more functional than 03 or 11 with similarly more complaint toolchains. For a math lib there's no real advantage to requiring say 20. We barely use the STL as it is.

Header Layout - https://github.com/cppalliance/decimal/issues/804

Correctness - Bugs are not uncommon during these reviews especially given the size of the library. I believe we have addressed, fixed, or in the process of fixing things you have brought up.

General Comments

Counting Digits - It's easy to say a binary search tree is a naive implementation. A CLZ based implementation from blog posts by Daniel Lemire and Junekey Jeon can be found on branch `better_count_dig`. I found it actually benchmarked worse on a number of platforms.

"Only interesting design question is if functions producing new value should be void returning and
modify inplace argument passed by reference(as for example std::ranges::sort does) or they
should be returning a value." - I don't see any advantage to this as it would be a serious departure from expectations and norms.

Taking decimal types by reference instead of by value - fundamentally the decimalXX types are std::uint32_t, std::uint64_t, and struct { uint64_t hi, uint64_t lo }. I don't think you'll see any performance improvements with those. Maybe for the fast types? Those are still reasonably small structs. We can try a few.

Thank you for your review and high-quality bug reports. 

Matt