Address review comments.

python · markshannon · Mar 22, 2023 · Jan 25, 2023 · Jan 25, 2023 · Feb 28, 2023
commit 7f5acc0eba93a4c5b38642792464eae927774e62
diff --git a/Include/internal/pycore_long.h b/Include/internal/pycore_long.h
@@ -118,11 +118,13 @@ PyAPI_FUNC(char*) _PyLong_FormatBytesWriter(
 #define SIGN_NEGATIVE 2
 #define NON_SIZE_BITS 3
 
-/* All "single digit" values are guaranteed to fit into
+/* All *compact" values are guaranteed to fit into
  * a Py_ssize_t with at least one bit to spare.
+ * In other words, for 64 bit machines, compact
+ * will be signed 63 (or fewer) bit values
  */
 
-/* Return 1 if the argument is positive single digit int */
+/* Return 1 if the argument is compact int */
 static inline int
 _PyLong_IsNonNegativeCompact(const PyLongObject* op) {
     assert(PyLong_Check(op));
@@ -142,7 +144,9 @@ _PyLong_BothAreCompact(const PyLongObject* a, const PyLongObject* b) {
     return (a->long_value.lv_tag | b->long_value.lv_tag) < (2 << NON_SIZE_BITS);
 }
 
-/* The value returned by this function will have at least one bit to spare,
+/* Returns a *compact* value, iff `_PyLong_IsCompact` is true for `op`.
+ *
+ * "Compact" values have at least one bit to spare,
  * so that addition and subtraction can be performed on the values
  * without risk of overflow.
  */
@@ -180,7 +184,7 @@ _PyLong_DigitCount(const PyLongObject *op)
     return op->long_value.lv_tag >> NON_SIZE_BITS;
 }
 
-/* Equivalent to _PyLong_DigitCount(op) * _PyLong_NonZeroSign(op) */
+/* Equivalent to _PyLong_DigitCount(op) * _PyLong_NonCompactSign(op) */
 static inline Py_ssize_t
 _PyLong_SignedDigitCount(const PyLongObject *op)
 {
@@ -199,9 +203,10 @@ _PyLong_UnsignedDigitCount(const PyLongObject *op)
 }
 
 static inline int
-_PyLong_NonZeroSign(const PyLongObject *op)
+_PyLong_NonCompactSign(const PyLongObject *op)
 {
     assert(PyLong_Check(op));
+    assert(!_PyLong_IsCompact(op));
     return 1 - (op->long_value.lv_tag & SIGN_MASK);
 }
 
@@ -215,7 +220,7 @@ _PyLong_SameSign(const PyLongObject *a, const PyLongObject *b)
 #define TAG_FROM_SIGN_AND_SIZE(sign, size) ((1 - (sign)) | ((size) << NON_SIZE_BITS))
 
 static inline void
-_PyLong_SetSignAndSize(PyLongObject *op, int sign, Py_ssize_t size)
+_PyLong_SetSignAndDigitCount(PyLongObject *op, int sign, Py_ssize_t size)
 {
     assert(size >= 0);
     assert(-1 <= sign && sign <= 1);
@@ -224,16 +229,18 @@ _PyLong_SetSignAndSize(PyLongObject *op, int sign, Py_ssize_t size)
 }
 
 static inline void
-_PyLong_SetSize(PyLongObject *op, Py_ssize_t size)
+_PyLong_SetDigitCount(PyLongObject *op, Py_ssize_t size)
 {
     assert(size >= 0);
     op->long_value.lv_tag = (((size_t)size) << NON_SIZE_BITS) | (op->long_value.lv_tag & SIGN_MASK);
 }
 
+#define NON_SIZE_MASK ~((1 << NON_SIZE_BITS) - 1)
+
 static inline void
 _PyLong_FlipSign(PyLongObject *op) {
     unsigned int flipped_sign = 2 - (op->long_value.lv_tag & SIGN_MASK);
-    op->long_value.lv_tag &= ~7;
+    op->long_value.lv_tag &= NON_SIZE_MASK;
     op->long_value.lv_tag |= flipped_sign;
 }
 

diff --git a/Misc/NEWS.d/next/Core and Builtins/2023-03-06-10-02-22.gh-issue-101291.0FT2QS.rst b/Misc/NEWS.d/next/Core and Builtins/2023-03-06-10-02-22.gh-issue-101291.0FT2QS.rst
@@ -1,5 +1,5 @@
 Rearrage bits in first field (after header) of PyLongObject. * Bits 0 and 1:
-1- sign. I.e. 0 for positive numbers, 1 for zero and 2 for negative numbers.
+1 - sign. I.e. 0 for positive numbers, 1 for zero and 2 for negative numbers.
 * Bit 2 reserved (probably for the immortal bit) * Bits 3+ the unsigned
 size.
 

diff --git a/Objects/longobject.c b/Objects/longobject.c
@@ -124,10 +124,10 @@ long_normalize(PyLongObject *v)
         --i;
     if (i != j) {
         if (i == 0) {
-            _PyLong_SetSignAndSize(v, 0, 0);
+            _PyLong_SetSignAndDigitCount(v, 0, 0);
         }
         else {
-            _PyLong_SetSize(v, i);
+            _PyLong_SetDigitCount(v, i);
         }
     }
     return v;
@@ -163,7 +163,7 @@ _PyLong_New(Py_ssize_t size)
         PyErr_NoMemory();
         return NULL;
     }
-    _PyLong_SetSignAndSize(result, size != 0, size);
+    _PyLong_SetSignAndDigitCount(result, size != 0, size);
     _PyObject_Init((PyObject*)result, &PyLong_Type);
     return result;
 }
@@ -180,7 +180,7 @@ _PyLong_FromDigits(int negative, Py_ssize_t digit_count, digit *digits)
         PyErr_NoMemory();
         return NULL;
     }
-    _PyLong_SetSignAndSize(result, negative?-1:1, digit_count);
+    _PyLong_SetSignAndDigitCount(result, negative?-1:1, digit_count);
     memcpy(result->long_value.ob_digit, digits, digit_count * sizeof(digit));
     return result;
 }
@@ -212,7 +212,7 @@ _PyLong_FromMedium(sdigit x)
         return NULL;
     }
     digit abs_x = x < 0 ? -x : x;
-    _PyLong_SetSignAndSize(v, x<0?-1:1, 1);
+    _PyLong_SetSignAndDigitCount(v, x<0?-1:1, 1);
     _PyObject_Init((PyObject*)v, &PyLong_Type);
     v->long_value.ob_digit[0] = abs_x;
     return (PyObject*)v;
@@ -246,7 +246,7 @@ _PyLong_FromLarge(stwodigits ival)
     PyLongObject *v = _PyLong_New(ndigits);
     if (v != NULL) {
         digit *p = v->long_value.ob_digit;
-        _PyLong_SetSignAndSize(v, sign, ndigits);
+        _PyLong_SetSignAndDigitCount(v, sign, ndigits);
         t = abs_ival;
         while (t) {
             *p++ = Py_SAFE_DOWNCAST(
@@ -319,7 +319,7 @@ PyLong_FromLong(long ival)
     v = _PyLong_New(ndigits);
     if (v != NULL) {
         digit *p = v->long_value.ob_digit;
-        _PyLong_SetSignAndSize(v, ival < 0 ? -1 : 1, ndigits);
+        _PyLong_SetSignAndDigitCount(v, ival < 0 ? -1 : 1, ndigits);
         t = abs_ival;
         while (t) {
             *p++ = (digit)(t & PyLong_MASK);
@@ -496,7 +496,7 @@ PyLong_AsLongAndOverflow(PyObject *vv, int *overflow)
     else {
         res = -1;
         i = _PyLong_DigitCount(v);
-        sign = _PyLong_NonZeroSign(v);
+        sign = _PyLong_NonCompactSign(v);
         x = 0;
         while (--i >= 0) {
             prev = x;
@@ -586,7 +586,7 @@ PyLong_AsSsize_t(PyObject *vv) {
         return _PyLong_CompactValue(v);
     }
     i = _PyLong_DigitCount(v);
-    sign = _PyLong_NonZeroSign(v);
+    sign = _PyLong_NonCompactSign(v);
     x = 0;
     while (--i >= 0) {
         prev = x;
@@ -721,11 +721,11 @@ _PyLong_AsUnsignedLongMask(PyObject *vv)
         return (unsigned long) -1;
     }
     v = (PyLongObject *)vv;
-    if (_PyLong_IsNonNegativeCompact(v)) {
+    if (_PyLong_IsCompact(v)) {
         return (unsigned long)_PyLong_CompactValue(v);
     }
     i = _PyLong_DigitCount(v);
-    int sign = _PyLong_NonZeroSign(v);
+    int sign = _PyLong_NonCompactSign(v);
     x = 0;
     while (--i >= 0) {
         x = (x << PyLong_SHIFT) | v->long_value.ob_digit[i];
@@ -764,7 +764,7 @@ _PyLong_Sign(PyObject *vv)
 
     assert(v != NULL);
     assert(PyLong_Check(v));
-    return _PyLong_NonZeroSign(v);
+    return _PyLong_NonCompactSign(v);
 }
 
 static int
@@ -918,7 +918,7 @@ _PyLong_FromByteArray(const unsigned char* bytes, size_t n,
     if (idigit == 0) {
         sign = 0;
     }
-    _PyLong_SetSignAndSize(v, sign, idigit);
+    _PyLong_SetSignAndDigitCount(v, sign, idigit);
     return (PyObject *)maybe_small_long(long_normalize(v));
 }
 
@@ -1139,7 +1139,7 @@ PyLong_FromLongLong(long long ival)
     v = _PyLong_New(ndigits);
     if (v != NULL) {
         digit *p = v->long_value.ob_digit;
-        _PyLong_SetSignAndSize(v, ival < 0 ? -1 : 1, ndigits);
+        _PyLong_SetSignAndDigitCount(v, ival < 0 ? -1 : 1, ndigits);
         t = abs_ival;
         while (t) {
             *p++ = (digit)(t & PyLong_MASK);
@@ -1182,7 
10000
+1182,7 @@ PyLong_FromSsize_t(Py_ssize_t ival)
     v = _PyLong_New(ndigits);
     if (v != NULL) {
         digit *p = v->long_value.ob_digit;
-        _PyLong_SetSignAndSize(v, negative ? -1 : 1, ndigits);
+        _PyLong_SetSignAndDigitCount(v, negative ? -1 : 1, ndigits);
         t = abs_ival;
         while (t) {
             *p++ = (digit)(t & PyLong_MASK);
@@ -1289,11 +1289,11 @@ _PyLong_AsUnsignedLongLongMask(PyObject *vv)
         return (unsigned long long) -1;
     }
     v = (PyLongObject *)vv;
-    if (_PyLong_IsNonNegativeCompact(v)) {
-        return _PyLong_CompactValue(v);
+    if (_PyLong_IsCompact(v)) {
+        return (unsigned long long)(signed long long)_PyLong_CompactValue(v);
     }
     i = _PyLong_DigitCount(v);
-    sign = _PyLong_NonZeroSign(v);
+    sign = _PyLong_NonCompactSign(v);
     x = 0;
     while (--i >= 0) {
         x = (x << PyLong_SHIFT) | v->long_value.ob_digit[i];
@@ -1366,7 +1366,7 @@ PyLong_AsLongLongAndOverflow(PyObject *vv, int *overflow)
     }
     else {
         i = _PyLong_DigitCount(v);
-        sign = _PyLong_NonZeroSign(v);
+        sign = _PyLong_NonCompactSign(v);
         x = 0;
         while (--i >= 0) {
             prev = x;
@@ -2473,7 +2473,7 @@ long_from_non_binary_base(const char *start, const char *end, Py_ssize_t digits,
         *res = NULL;
         return 0;
     }
-    _PyLong_SetSignAndSize(z, 0, 0);
+    _PyLong_SetSignAndDigitCount(z, 0, 0);
 
     /* `convwidth` consecutive input digits are treated as a single
      * digit in base `convmultmax`.
@@ -2525,7 +2525,7 @@ long_from_non_binary_base(const char *start, const char *end, Py_ssize_t digits,
             if (_PyLong_DigitCount(z) < size_z) {
                 *pz = (digit)c;
                 assert(!_PyLong_IsNegative(z));
-                _PyLong_SetSignAndSize(z, 1, _PyLong_DigitCount(z) + 1);
+                _PyLong_SetSignAndDigitCount(z, 1, _PyLong_DigitCount(z) + 1);
             }
             else {
                 PyLongObject *tmp;
@@ -3279,7 +3279,7 @@ long_hash(PyLongObject *v)
         return x;
     }
     i = _PyLong_DigitCount(v);
-    sign = _PyLong_NonZeroSign(v);
+    sign = _PyLong_NonCompactSign(v);
     x = 0;
     while (--i >= 0) {
         /* Here x is a quantity in the range [0, _PyHASH_MODULUS); we
@@ -3877,7 +3877,7 @@ k_lopsided_mul(PyLongObject *a, PyLongObject *b)
         memcpy(bslice->long_value.ob_digit, b->long_value.ob_digit + nbdone,
                nbtouse * sizeof(digit));
         assert(nbtouse >= 0);
-        _PyLong_SetSignAndSize(bslice, 1, nbtouse);
+        _PyLong_SetSignAndDigitCount(bslice, 1, nbtouse);
         product = k_mul(a, bslice);
         if (product == NULL)
             goto fail;
@@ -3938,7 +3938,7 @@ fast_mod(PyLongObject *a, PyLongObject *b)
 
     assert(_PyLong_DigitCount(a) == 1);
     assert(_PyLong_DigitCount(b) == 1);
-    sdigit sign = _PyLong_NonZeroSign(b);
+    sdigit sign = _PyLong_NonCompactSign(b);
     if (_PyLong_SameSign(a, b)) {
         mod = left % right;
     }
@@ -4966,7 +4966,7 @@ long_rshift1(PyLongObject *a, Py_ssize_t wordshift, digit remshift)
             significant `wordshift` digits of `a` is nonzero. Digit `wordshift`
             of `2**shift - 1` has value `PyLong_MASK >> hishift`.
         */
-        _PyLong_SetSignAndSize(z, -1, newsize);
+        _PyLong_SetSignAndDigitCount(z, -1, newsize);
 
         digit sticky = 0;
         for (Py_ssize_t j = 0; j < wordshift; j++) {
@@ -5387,7 +5387,7 @@ _PyLong_GCD(PyObject *aarg, PyObject *barg)
         }
         if (c != NULL) {
             assert(size_a >= 0);
-            _PyLong_SetSignAndSize(c, 1, size_a);
+            _PyLong_SetSignAndDigitCount(c, 1, size_a);
         }
         else if (Py_REFCNT(a) == 1) {
             c = (PyLongObject*)Py_NewRef(a);
@@ -5401,12 +5401,12 @@ _PyLong_GCD(PyObject *aarg, PyObject *barg)
 
         if (d != NULL) {
             assert(size_a >= 0);
-            _PyLong_SetSignAndSize(d, 1, size_a);
+            _PyLong_SetSignAndDigitCount(d, 1, size_a);
         }
         else if (Py_REFCNT(b) == 1 && size_a <= alloc_b) {
             d = (PyLongObject*)Py_NewRef(b);
             assert(size_a >= 0);
-            _PyLong_SetSignAndSize(d, 1, size_a);
+            _PyLong_SetSignAndDigitCount(d, 1, size_a);
         }
         else {
             alloc_b = size_a;

diff --git a/Python/marshal.c b/Python/marshal.c
@@ -840,7 +840,7 @@ r_PyLong(RFILE *p)
     if (ob == NULL)
         return NULL;
 
-    _PyLong_SetSignAndSize(ob, n < 0 ? -1 : 1, size);
+    _PyLong_SetSignAndDigitCount(ob, n < 0 ? -1 : 1, size);
 
     for (i = 0; i < size-1; i++) {
         d = 0;

diff --git a/Tools/build/umarshal.py b/Tools/build/umarshal.py
@@ -94,8 +94,6 @@ def __init__(self, data: bytes):
         self.level: int = 0
 
     def r_string(self, n: int) -> bytes:
-        if not (0 <= n <= self.end - self.pos):
-            print(n, self.end, self.pos)
         assert 0 <= n <= self.end - self.pos
         buf = self.data[self.pos : self.pos + n]
         self.pos += n