Math.gcd new function by jbbjarnason · Pull Request #8331 · micropython/micropython (original) (raw)

Hello jbbjarnason,

out of curiosity I had a look into this commit ( I want to learn how to contribute to MPy and the gcd topic seems simple).
And I noticed you decided for an algorithm which is mathematically simple and correct but may lead to endless recursion if you e.g. compute gcd(2147483643, 42). Even under Linux this leads to Memory Error.

I tested the following algorithms:

#include <stdio.h>

int gcd_func(int x, int y) {
    if (x == 0) {
        return y;
    }
    if (y == 0) {
        return x;
    }
    if (x == y) {
        return x;
    }
    if (x > y) {
        return gcd_func(x - y, y);
    }
    return gcd_func(x, y - x);
}

int gcd_spl(int u, int v)
{
    int t;

    if (u < 0)
        u = -u;
    if (v < 0)
        v = -v;
    if (v > u) {   // swap u and v
        t = u;
        u = v;
        v = t;
    }
    while ((t = u % v) > 0) {
        u = v;
        v = t;
    }

    return v;
}

int gcd_iter(int u, int v)
{
    if (u < 0) u = -u;
    if (v < 0) v = -v;
    while ((u %= v) && (v %= u))
        ;
    return (u + v);
}

int main()
{
    int (*gcd)(int, int);
    gcd = gcd_func;
//	gcd = gcd_iter;
//	gcd = gcd_spl;

    printf("gcd(112, 6) = %d\n", gcd(112, 6));
    printf("gcd(112, -6) = %d\n", gcd(112, -6));
    printf("gcd(-112, 6) = %d\n", gcd(-112, 6));
    printf("gcd(-112, -6) = %d\n", gcd(-112, -6));
    printf("gcd(2147483643, 42) = %d\n", gcd(2147483643, 42));
    
    return 0;
}

gcd_iter() is from Rosetta code http://www.rosettacode.org/wiki/Greatest_common_divisor#C.
It has unnecessary many remainder (%) - computations to keep the code short.
I simplified/straightened it to gcd_spl().
gcd_func() is your code in plain c.
If you run this code you notice the problems.
I suggest to adapt the Pull-request to contain something like gcd_spl().
I still could not do that, as I don't know github well enough and don't know how to compute with big ints in MPy internals.
I also suggest to include computation of gcd(2147483643, 42) and even an example with a larger (bigint) number into the tests.

Greetings,
Raul