[decimal] Late review on Boost.Decimal
Hi all, This is my (first) review of the Boost.Decimal library. First, I'd like to express my appreciation to Matt and Chris for their considerable work and dedication in developing this valuable library. <Background> I know the basics of IEEE 754 floating point. Since September, I've been experimenting with Boost.Decimal. I experimented with the library for several days both during the pre-review period and again during the main review period. <Initial experiments> My initial experience with the library was very smooth. The documentation offered useful examples and I was able to build and run without any problems on GCC, Clang, and Apple Clang. <Documentation> As I became more familiar with the library, I started to feel that the documentation was somewhat lacking. Some sections (e.g. `decimalXX_t`, literal support, <cstdlib> support, …) consist mostly of synopses with little explanatory text. As a result, I sometimes had to write and run small pieces of code to understand detailed behavior. It might be easier to follow if related topics were grouped together in the same section. For example, it could be clearer to place items such as - `from_chars` and `to_chars` (<charconv> support) - `strtod` family (<stdlib> support) under a common section like "IO and String Conversions". Early on, the links to the subsections in "Examples" on the left-side TOC were helpful. As I began using the later sections more, listing every Examples' subsection slowed access to them. Showing only a subset by default might improve navigation. This is a minor and subjective point. <Tests> Shortly after starting to use the library, I found a few bugs. While Matt handled the issue reports remarkably quickly, which I greatly appreciated, it was a bit surprising that they could be found easily. This suggests that the current test coverage might still be somewhat limited. As I mentioned in the main thread, using the decTest (General Decimal Arithmetic Testcases) could help identify such issues more systematically and improve the library's reliability. When using decTest, a few practical points may be worth noting: - The files prefixed with `ds`, `dd`, and `dq` correspond to decimal32, decimal64, and decimal128. - The decTest testcases are decarith-flavored (General Decimal Arithmetic Specification), meaning that they include behavior that goes beyond the IEEE 754 standard, such as additional rounding modes. Therefore, for example, testcases that use non-IEEE 754 rounding mode should be skipped, and testcases whose results are NaN should be compared in a relaxed manner (i.e. the comparison should ignore the sign and the payload). - The `ds…` testcases provide very limited coverage; porting testcases from `dd…` to decimal32 is likely more effective. - The bit patterns used in decTest are in DPD encoding. Since the testcases lack BID-specific edge cases, it would be useful to add testcases for non-canonical zeros (i.e. cases where the coefficient exceeds 10^p). For instance, when running `ddAdd.decTest`, 65 out of 971 testcases fail when comparing numerical results, and 286 out of 971 fail when comparing results bit by bit, which also verifies that the selected quantum is appropriate. <Conformance to IEEE 754> I understand that conformance to IEEE 754 may not be the library's primary goal, so this point might be less relevant. Still, I list conformance-related points below, since I personally believe that any library claiming to implement IEEE 754 should conform to the standard (possibly with very limited exceptions): * The default rounding mode is not the recommended one. - This is not a conformance violation, as the choice of the default rounding is only a recommendation. - **Fixed**. The default rounding mode is now the recommended one. * The mandatory operations do not satisfy correct rounding (CR). - **Documented** as a design decision. * FP exceptions are not supported. - **Documented** as a design decision. * For certain operations, the preferred quantum is not used. - This should be detectable with decTest. * Some IEEE 754 mandatory operations are not implemented. - In addition, `convertToInteger` family should round using a mode indicated by the function names (using a rounding mode parameter may also acceptable as in C23) rather than using the current rounding mode. Here are a few comments on the two design choices noted above: * CR on mandatory operations - It would help to document the downsides of not providing CR: * Even when using *reproducible* operations (see e.g. IEEE 754 Clause 11), results may diverge from CR libraries (e.g. Intel's decimal library). If a compiler's decimal FP backend uses such a CR library, results may also diverge from the C23 standard library. * When operations are not correctly rounded, the set of permissible results may contain more than one value. Therefore, even when limited to *reproducible* operations, different Boost.Decimal versions may produce different results. Adopting a faster algorithm, or later switching to a CR implementation, may change earlier results. - It would be helpful to document which operations are CR, including arithmetic operations and string-decimal conversions. * FP exception - It would also be beneficial to spell out what is not possible without FP exceptions. For example, callers cannot tell when rounding occurred. <Conclusion> I recommend **Conditional Accept**, on the condition that a setup is provided to run decTest regularly (e.g. as part of CI). As a wish list that is not a requirement, I would like to suggest the following: - The decTest results should be reasonably green; at least for the very basic operations, all testcases should pass. - It would be helpful to provide a mapping table between the IEEE 754 mandatory operations and the corresponding Boost.Decimal APIs, similar to the one found in C23 Clause F.2 or Section 9 of the Intel Decimal Floating-Point Math Library README. With these in place, I think that the library's conformance will be easier to assess and ongoing maintenance will be simpler over time. Thanks again to Matt and Chris for developing this valuable library, and to John for managing this review. Regards, Michel
It might be easier to follow if related topics were grouped together in the same section. For example, it could be clearer to place items such as - `from_chars` and `to_chars` (<charconv> support)
- `strtod` family (<stdlib> support)
under a common section like "IO and String Conversions".
Based off of the comments there will be another round of re-organization. I thought that having every function under it's analogous STL header, but that has clearly been shown to not be the case.
Here are a few comments on the two design choices noted above:
* CR on mandatory operations - It would help to document the downsides of not providing CR: * Even when using reproducible operations (see e.g. IEEE 754 Clause 11), results may diverge from CR libraries (e.g. Intel's decimal library). If a compiler's decimal FP backend uses such a CR library, results may also diverge from the C23 standard library.
The Intel Library's README is not correctly rounded for all operations either, and states as much (which I quote in the docs). I can add in a line that says results will differ in the last few ULPs. You will find that even between standard library implementations for binary-floating point math results will vary outside of 0.5 ULPs.
<Conclusion>
I recommend Conditional Accept, on the condition that a setup is provided to run decTest regularly (e.g. as part of CI).
Can you share the test harness that you've already built? I never put effort into them because the copyright All Rights Reserved IBM precludes them from being checked into the repo or modified like your previous comments call out. I don't really want to spam downloading from Mike's website, but he at least has the disclaimer of reproduced with permission from IBM. Thank you for your review and working with us on bug reports in the weeks leading up to the review! Matt
Matt Borland wrote:
* CR on mandatory operations ... The Intel Library's README is not correctly rounded for all operations either, and states as much (which I quote in the docs). I can add in a line that says results will differ in the last few ULPs. You will find that even between standard library implementations for binary-floating point math results will vary outside of 0.5 ULPs.
To be precise, I'm referring only to "mandatory operations" in IEEE 754-2019 (Clause 5). "Optional/extended operations" are out of scope here. The README says that the library conforms to IEEE 754-2019. By definition, this implies that mandatory operations are CR. If you've observed a non-CR result for a mandatory operation, that's likely a bug, and it would be helpful to report an issue.
Can you share the test harness that you've already built?
I'm happy to share my harness after I take the time to brush it up, but that may take a while. That said, I think crafting a new harness is straightforward and takes minimal effort.
I never put effort into them because the copyright All Rights Reserved IBM precludes them from being checked into the repo or modified like your previous comments call out. I don't really want to spam downloading from Mike's website, but he at least has the disclaimer of reproduced with permission from IBM.
The website (https://www.speleotrove.com/decimal/) says that the testcases are also covered by the ICU license. Regarding upstream bandwidth during CI, we can use caching in GitHub Actions, so we don't need to fetch the files on every run. Once the cache is populated, later jobs restore from the cache instead of downloading, which keeps upstream traffic to a minimum. Regards, Michel
participants (2)
-
Matt Borland -
Michel Morin