[boost][decimal] LoS's review
Dear Boosters, I'd first like to thank Matt for his effort in designing and implementing the library. Below, there's my review of the "Boost.Decimal" library. ## Documentation An issue is the inconsistent use of `using namespace` declarations in the code snippets. In some examples, the global declaration allows to write types simply as `decimal32_t`; in others, the examples omit the declaration, forcing to repeatedly write the full namespace, such as `boost::decimal::decimal32_t`. I think it'd be better to keep a consistent style throughout the documentation. The current documentation often lacks necessary detail for functions. While the synopsis and a generic description of the main classes (decimal32_t, decimal64_t, decimal128_t) are present, there's lack of information, like preconditions or edge-case behaviors, for many functions. Some examples aren't embedded directly in the documentation, but are merely mentioned as external files. While this prevents large blocks of code from cluttering the documentation, it imposes a burden on the user, who must manually navigate the repository to find these files. A much better solution would be to provide direct inks to these files. In this way, users could quickly jump to the code and return to the documentation, without the need to open a new browser window and manually look for the specific files. Finally, the "API Reference" section organizes some information using tables. While the tabular format is appropriate for structures where multiple fields must be summarized, its use to some types, like enums, seems arbitrary. Also, the visual display of these tables can be confusing upon first glance, since it may lead the user to incorrectly assume a relationship between adjacent, but functionally separate, tables. ## Design I think the use of explicit constructors should be more clearly defined. Some constructors are marked explicit, while others are not, and the reasoning for this difference isn't immediately clear. The "Design notes" section explains the decision to make constructors for floating-point numbers explicit. However, it omits any rationale for why constructors for integer types have been made non-explicit. I think that a complete explanation of the rationale behind constructor explicitness would be beneficial. About the documentation, the constructors, for both floating-point numbers and integers, are marked as explicit. This mismatch should be fixed, too. Best regards, LoS
The current documentation often lacks necessary detail for functions. While the synopsis and a generic description of the main classes (decimal32_t, decimal64_t, decimal128_t) are present, there's lack of information, like preconditions or edge-case behaviors, for many functions.
Andrzej opened an issue tracking this towards the beginning of the review period. It'll be fixed.
Some examples aren't embedded directly in the documentation, but are merely mentioned as external files. While this prevents large blocks of code from cluttering the documentation, it imposes a burden on the user, who must manually navigate the repository to find these files. A much better solution would be to provide direct inks to these files. In this way, users could quickly jump to the code and return to the documentation, without the need to open a new browser window and manually look for the specific files.
Fair enough, and relatively easy to change.
I think the use of explicit constructors should be more clearly defined.
Yes, and the integer ones will be all marked explicit. Do you care to explicitly specify if you want the library accepted or rejected? You do not have to if you do not want to. Either way thank you for the comments. Matt
participants (2)
-
LoS -
Matt Borland