Skip to main content
2 of 5
Fix << -ve
Toby Speight
  • 88.4k
  • 14
  • 104
  • 327

Naming

The established convention in C and C++ is that we use all-caps names for preprocessor macros and not for other identifiers. This helps alert readers to these textual transformations that don't obey the rules of scope and precedence we expect from the language.

The macros PRT() and PRTB() conform to this convention, but macros cmath_ver and cmath_ret do not, nor the namespace ALFPN, nor parameter N to mask_lowest_N_bits(), nor variables RMNDR_MASK, RNDGBIT_MASK, etc. That dilutes the value of the convention and makes life harder for readers.

Includes

We use types such as size_t, uint_least64_t, etc without including <stdint.h>, which is an error. That said, I would include instead and change those usages to std::size_t, std::uint_least64_t, etc.

Also missing is <climits>, to provide definition of CHAR_BIT.

Utility functions

safe_shift_left() and safe_shift_right() both fail to handle negative shifts correctly. We could fix that, but probably simpler to remove these functions and inline into safe_shift(), which is the only one that's actually called:

        template<std::integral T>
        constexpr T safe_shift(T val, int shift)
        {
            constexpr auto bit_width = sizeof val * CHAR_BIT;

            if (shift <= -bit_width) {
                // overshift right
                return val >> (bit_width - 1);
            } else if (shift < 0) {
                return val >> -shift;
            } else if (shift < bit_width) {
                return val << shift;
            } else {
                // overshift left
                return 0;
            }
        }

The computation sizeof (T) * CHAR_BIT occurs in several places; it might be worth writing a small template for this:

        template<typename T>
        int type_width = sizeof (T) * CHAR_BIT;

(Not called bit_width, to avoid confusion with standard-library function of that name)

Standard warning about possible overflow in cexpr_exp2() when n == INT_MIN. We might be able to prove that would never happen, but then we should have a comment justifying that. Or we could avoid the issue altogether, using result /= for negative n.

mask_lowest_N_bits() is unclear, and the comments aren't especially helpful. In particular, I cannot see why the static_cast of the return value from UT to UT is needed.

I think the intent is that for n = 0, 1, 2, 3, we should return 0, 0b1, 0b11, 0b111, respectively. The usual technique for that would be to form an all-ones value, shift that left by n bits, then complement it:

        template <typename T>
        constexpr T mask_lowest_N_bits(std::size_t N)
        {
            return ~safe_shift(~T{}, n);
        }

This is a partial review; I hope to continue from line 159 later.

Toby Speight
  • 88.4k
  • 14
  • 104
  • 327