@yoctopuce suggested we test MICROPY_OBJ_REPR_C and MICROPY_LONGINT_IMPL_LONGLONG together, which makes sense - and I don't think we have CI coverage for the former at the moment.

@dpgeorge what do you think?

Yes, that's a very good idea to test MICROPY_OBJ_REPR_C in CI.

It's enabled on esp8266 and stm32 Arduino boards, so it does get tested for a release. But would be great to test it in CI.

Enabling representation C in this new longlong variant is a good idea!

Yeah, it's a bit of a nuisance to need to include this header... it sometimes doesn't exist on embedded systems.

At the very least I'd suggest moving this include to within the MICROPY_LONGINT_IMPL == MICROPY_LONGINT_IMPL_LONGLONG guard, so it's only included if needed (because this file will be compiled on all systems regardless of the config).

In the long run, would be best to remove dependency on errno, but I'm not sure it's worth the effort given this config is likely not used much.

I've moved it down behind the preprocessor guard.

I did start writing a version that doesn't depend on strtoll, using the approach @yoctopuce suggested for mp_parse_num_integer() to handle 64-bit. It has potential to work quite nicely, but it wasn't particularly simple either so I shelved it for now.

Here is my version, in case it helps:

    const byte* restrict str = (const byte*)*str_;
    const byte* restrict top = str + len;
    long long int_val = 0;
    for (; str < top; str++) {
        // get next digit as a value
        mp_uint_t dig = *str;
        if ('0' <= dig && dig <= '9') {
            dig -= '0';
        } else if (dig == '_') {
            continue;
        } else {
            dig |= 0x20; // make digit lower-case
            if ('a' <= dig && dig <= 'z') {
                dig -= 'a' - 10;
            } else {
                // unknown character
                break;
            }
        }
        if (dig >= (mp_uint_t)base) {
            break;
        }

        // add next digit
        int_val = int_val * base + dig;
    }

    // negate value if needed
    if (neg) {
        int_val = -int_val;
    }
    mp_obj_t result = mp_obj_new_int_from_ll(int_val);
    *str_ = (char*)str;

I got inspired to go and finish the version of this I was working on, have updated this PR with the result.

I think this needs to be wrapped in #if defined __has_builtin. See py/misc.h and https://gcc.gnu.org/onlinedocs/cpp/_005f_005fhas_005fbuiltin.html

Or follow lib/cmsis/inc/cmsis_gcc.h:

#ifndef __has_builtin
  #define __has_builtin(x) (0)
#endif

I also hit that __has_builtin issue when integrating this commit in my dev branch, and as __has_builtin() is sometime undefined on compilers that do have support for __builtin_*_overflow, I ended up using this code:

#if defined(_MSC_VER)
#define ENABLE_INTSAFE_SIGNED_FUNCTIONS
#include <intsafe.h>
#define add_overflow(lhs,rhs,res)   LongLongAdd(lhs, rhs, res)
#define sub_overflow(lhs,rhs,res)   LongLongSub(lhs, rhs, res)
#define mul_overflow(lhs,rhs,res)   LongLongMult(lhs,rhs,res)
#elif defined(__EMSCRIPTEN__ ) || (defined(__GNUC__) && __GNUC__ >= 5)
#define add_overflow(lhs,rhs,res)   __builtin_add_overflow(lhs, rhs, res)
#define sub_overflow(lhs,rhs,res)   __builtin_sub_overflow(lhs, rhs, res)
#define mul_overflow(lhs,rhs,res)   __builtin_mul_overflow(lhs, rhs, res)
#endif

#if !defined(add_overflow)
#include <limits.h>

__inline bool add_overflow(long long lhs, long long rhs, long long *result) {
    // Check for overflow
    if ((rhs > 0) && (lhs > LLONG_MAX - rhs)) {
        return 1; // Overflow
    }
    if ((rhs < 0) && (lhs < LLONG_MIN - rhs)) {
        return 1; // Overflow
    }
    *result = lhs + rhs;
    return 0; // No overflow
}

__inline bool sub_overflow(long long lhs, long long rhs, long long *result) {
    // Check for overflow
    if ((rhs > 0) && (lhs < LLONG_MIN + rhs)) {
        return 1; // Overflow
    }
    if ((rhs < 0) && (lhs > LLONG_MAX + rhs)) {
        return 1; // Overflow
    }
    *result = lhs - rhs;
    return 0; // No overflow
}

__inline bool mul_overflow(long long lhs, long long rhs, long long * result)
{
    if (lhs > 0) {
        if (rhs > 0) {
            if (lhs > (LLONG_MAX / rhs)) {
                return 1; // Overflow
            }
        } else {
            if (rhs < (LLONG_MIN / lhs)) {
                return 1; // Overflow
            }
        }
    } else {
        if (rhs > 0) {
            if (lhs < (LLONG_MIN / rhs)) {
                return 1; // Overflow
            }
        } else {
            if ((lhs != 0) && (rhs <= (LLONG_MAX / lhs))) {
                return 1; // Overflow
            }
        }
    }
    *result = lhs * rhs;
    return 0; // No overflow
}
#endif

My embedded platforms are all using GCC >= 5, so the bigger+slower inline code only ends up in OS-based test builds.

Impressive! @dpgeorge do you think it's worth pulling any/all of this into MP, or stick with __has_builtin and fallback to not checking overflow? (__has_builtin is GCC 10 and up, first released 2020.)

(I've added the check for defined __has_builtin already.)

If we rely only on __has_builtin, we are going to significantly increase code size for all compiler version between 2015 ( GCC 5+) and 2020 (GCC 10+).

As __builtin_*_overflow functions have been officially included in GCC 5, you might want to override the default "not supported" setting by checking (defined(__GNUC__) && __GNUC__ >= 5)

The other option is to disable the overflow check on those compiler versions (which is what the earlier version of this PR did, although the latest version disables the overflow check for some options and keeps it for multiply due to integer parsing.)

@yoctopuce I don't suppose you have any measurement of the difference? There are some cases where the compiler can peephole optimise in a native overflow check from equivalent C code (but unsure if this is one of those cases). EDIT: Otherwise I will measure this difference next week.