Re: Greatest Common Divisor

On 29/12/2019 08:30, Fabien COELHO wrote:
>
>>> I'm wondering what it should do on N, 0 and 0, 0. Raise an error?
>>> Return 0? Return 1? return N? There should be some logic and comments
>>> explaining it.
>>
>> Well, gcd(N, 0) is N, and gcd(0, 0) is 0, so I don't see an issue here?
>
> I think that there should be a comment.


Done.


>>> It does not seem that fixing the sign afterwards is the right thing to
>>> do. I'd rather turn both arguments positive before doing the descent.
>>
>> Why isn't it the right thing to do?
>
> Because I do not trust C modulo as I had a lot of problems with it? :-)
>
> If it works, but it should deserve a clear comment explaining why.


Surely such a comment should be on the mod functions and not in this patch.


>
>>> Which raises an issue with INT_MIN by the way, which has no positive:-(
>>
>> That's an argument against abs-ing the input values.  It's only an issue
>> with gcd(INT_MIN, INT_MIN) which currently returns INT_MIN.
>
> That should be an error instead, because -INT_MIN cannot be represented?


Why should it error?  Is INT_MIN not a valid divisor of INT_MIN?  I
added a comment instead.


>>> Also, the usual approach is to exchange args so that the largest is
>>> first, because there may be a software emulation if the hardware does
>>> not implement modulo. At least it was the case with some sparc
>>> processors 25 years ago:-)
>>
>> The args will exchange themselves.
>
> Yep, but after a possibly expensive software-emulated modulo operation?
>

I'll just trust you on this.  Swap added.


Thanks!

-- 

Vik