Speedup and proper fix for gap conversion from sage/python integer

[user202729]

follow up to #41057

My machine has sizeof(mp_limb_t) == sizeof(UInt) true, but I've tested both branches by changing the if ... to if False and see that the tests pass.

the expression num_gmp_limbs * sizeof(mp_limb_t) cannot overflow (because there's a buffer in memory with that size), so as long as sizeof(mp_limb_t) > 1 (which must be the case, see below), -num_gmp_limbs cannot overflow UInt.

note on word size

From gmp-h.in:

#ifdef __GMP_SHORT_LIMB
typedef unsigned int		mp_limb_t;
typedef int			mp_limb_signed_t;
#else
#ifdef _LONG_LONG_LIMB
typedef unsigned long long int	mp_limb_t;
typedef long long int		mp_limb_signed_t;
#else
typedef unsigned long int	mp_limb_t;
typedef long int		mp_limb_signed_t;
#endif
#endif
typedef unsigned long int	mp_bitcnt_t;

/* For reference, note that the name __mpz_struct gets into C++ mangled
   function names, which means although the "__" suggests an internal, we
   must leave this name for binary compatibility.  */
typedef struct
{
  int _mp_alloc;		/* Number of *limbs* allocated and pointed
				   to by the _mp_d field.  */
  int _mp_size;			/* abs(_mp_size) is the number of limbs the
				   last field points to.  If _mp_size is
				   negative this is a negative number.  */
  mp_limb_t *_mp_d;		/* Pointer to the limbs.  */
} __mpz_struct;

From gap/src/common.h:

typedef intptr_t  Int;
typedef uintptr_t UInt;

so their sizes need not always be equal.

note on gap internal implementation of integer

That said, gap/src/integer.c contains this

GAP_STATIC_ASSERT( sizeof(mp_limb_t) == sizeof(UInt), "gmp limb size incompatible with GAP word size");

maybe we're safe...? On the other hand it's entirely possible that gap and sage are compared with different settings of gmp, they're distinct shared libraries.

also there's this

/****************************************************************************
**
**  'ADDR_INT' returns a pointer to the limbs of the large integer 'obj'.
**  'CONST_ADDR_INT' does the same, but returns a const pointer.
*/
EXPORT_INLINE UInt * ADDR_INT(Obj obj)
{
    GAP_ASSERT(IS_LARGEINT(obj));
    return (UInt *)ADDR_OBJ(obj);
}


static Obj GMPorINTOBJ_MPZ( mpz_t v )
{
    return MakeObjInt((const UInt *)v->_mp_d, v->_mp_size);
}

the former is actually exported. If we're fine with using more internals, we could call NewBag directly with the correct size, but I think that's unnecessary.

📝 Checklist

• The title is concise and informative.
• The description explains in detail what this PR is about.
• I have linked a relevant issue or discussion.
• I have created tests covering the changes.
• I have updated the documentation and checked the documentation preview.

⌛ Dependencies

[tobiasdiez]

Please do consider moving those tests to pytest. Now you simple get an assertion error in case of an issue without any details on what was the right and left side; pytests prints out a meaningful exception message in this case.

Moreover, tests using sage integers and using .sage() are now missing.

[user202729]

and make the test 10x or so more verbose? I disagree.

(there is a compromise, use something like https://docs.pytest.org/en/latest/reference/reference.html#pytest.register_assert_rewrite and inject it into the doctesting framework)

About .sage(), it's not the responsibility of this method, it's the responsibility of .sage(). There's already this

        TESTS::

            sage: libgap(0).sage()
            0
            sage: large = libgap.eval('2^130');  large
            1361129467683753853853498429727072845824
            sage: large.sage()
            1361129467683753853853498429727072845824

            sage: huge = libgap.eval('10^9999');  huge     # gap abbreviates very long ints
            <integer 100...000 (10000 digits)>
            sage: huge.sage().ndigits()
            10000

If .sage() and str agree, and str and libgap() agree, .sage() and libgap() must be equal.

[tobiasdiez]

and make the test 10x or so more verbose?

It will be the same code plus the import of libgap...

About .sage(), it's not the responsibility of this method, it's the responsibility of .sage().

Okay.

The sage-integer test is still missing, right?

[user202729]

that one is above (which is appropriate, this function isn't responsible for converting Sage integers)

        sage: for x in [0, 1, 2**31, 2**32, 2**63, 2**64, 2**128]:
        ....:     for y in [x, -x, x-1]:
        ....:         assert str(libgap(y)) == str(y), y

[cxzhong]

I think in this case we use sage_sig_free is much safer?

[user202729]

It isn't