- 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?
We use the unsigned overload extensively for numeric constants in the implementation of <cmath> functions. Not all platforms have (un)signed __int128 so we have a software u128 type that is allowed as an UnsignedInteger. This is important for decimal128_t constants.
- 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.
I can probably go through and simplify a bunch of these now. In the formative stages it made better logical sense to me using binary literal masks, even though they tend to be ugly. My comments are written out by the bit which is what I was going off of [1].
- 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.
I can't remember why I added it, but the build is clean without so I'll just delete it.
Conclusion
Thank you for your review and comments. Matt [1] https://github.com/cppalliance/decimal/blob/7d44a84c3fc271e48b6850a6b5a39f83...