Hi everyone, here's my review of the proposed Decimal library. First off, I'd like to thank Matt and Chris for the substantial work they've put into this submission—it's no small feat to bring such a comprehensive decimal implementation to the table. And thanks to John for stepping up as review manager. I spent some time exploring the documentation and found it generally well-structured and effective at onboarding users. That said, a few facilities appear to be undocumented (see below), and I spotted some minor issues in the code that—while not critical—could benefit from cleanup or clarification. Here's a rundown of my findings: API Design - Considering e.g. decimal32_t, I'm unsure about the rationale behind having both of these constructors: template <typename UnsignedInteger, typename Integer> constexpr decimal32_t(UnsignedInteger coefficient, Integer exponent, bool sign = false) noexcept; template <typename SignedInteger, typename Integer> constexpr decimal32_t(SignedInteger coefficient, Integer exponent) noexcept; The second seems sufficient. What's the intended use case for the first? Documentation - In the section titled "Fundamental Operations", the phrase "The fundamental operations of numerical type (e.g. >, ==, +, etc.) are overloaded." should actually say "numerical types". - In Examples -> Construction -> Literals and Constants, the statement “numeric_limits is overloaded for all decimal types” should say “specialized” instead of "overloaded". - The sentence "This example shows how to parse historical stock data from file and use it" would read more naturally as "from a file". - The navigation panel contains a typo: "Formating support" should be "Formatting support". - Title case in the navigation panel is used inconsistently (see the entries ending with "support".) - On the six pages dedicated to the six decimal types, there are notes saying "This support has been removed in v6.0.0". I think they would be better as "This name has been removed". You might omit the notes altogether if the library is accepted into Boost. Implementation details - The specializations of `std::numeric_limits` use conditional compilation to select between `class` and `struct`, and to specify `public:` access: template <> #ifdef _MSC_VER class numeric_limits<boost::decimal::decimal64_t> #else struct numeric_limits<boost::decimal::decimal64_t> #endif { #ifdef _MSC_VER public: #endif I think just using `class` and `public:` across all compilers is simpler and effective. - In decimal128_t.hpp, the file "detail/int128.hpp" is included twice. I suggest ordering the header-names alphabetically to prevent these kinds of oversights. - Bitwise operators are defined but don't seem to be documented. A rationale for their inclusion would be nice too. - Several binary literals throughout the codebase are difficult to read. For instance: UINT64_C(0b10000000000000000000000000000000000000000000000000) might become: UINT64_C(1) << 49 increasing readability and reducing visual noise. - In decimal128_t.hpp, the following diagnostic suppression block appears: #if defined(__GNUC__) && __GNUC__ >= 6 # pragma GCC diagnostic push # pragma GCC diagnostic ignored "-Wduplicated-branches" # pragma GCC diagnostic ignored "-Wconversion" #endif The presence of `-Wduplicated-branches` raises questions. Are there actual duplicated branches in the code? Or are you just suppressing some false positives? Clarifying this in a comment, or restructuring the code to avoid the warning, would be IMHO useful. Conclusion Overall, I found only minor issues in the documentation and code. I haven't reviewed the core decimal logic in depth, as I'm not deeply familiar with the decimal aspects of IEEE 754—but I trust others will scrutinize that part more thoroughly. Given the quality of the work and the minor nature of the issues I encountered, I believe Decimal deserves to be accepted into Boost. Thanks again to the authors and reviewers for their efforts. -- Gennaro Prota <https://prota.dev>