Re: Greatest Common Divisor - Mailing list pgsql-hackers

Hello Vik,

>> Add unlikely() where appropriate.
>
> Any particular place in mind where I didn't already put it?

In GCD implementations, for instance:

  if (arg1 == PG_INT32_MIN)
  if (arg2 == 0 || arg2 == PG_INT32_MIN)

And possibly a "likely" on the while.

In LCM implementations, for instance:

  if (arg1 == 0 || arg2 == 0)
  if (arg1 == arg2)

The later is partially redundant with preceeding case BTW, which could be 
managed inside this one, reducing the number of tests? Something like:

  if (arg1 == arg2)
    if (arg1 == MIN_INT)
      error
    else
      return abs(arg1)

I'm not sure why you want to deal with a1 == a2 case separately, could it 
not just work with the main code?

If you want to deal with it separately, then why not doing arg1 == -arg2 
as well?

> Please stop suggesting it.

Fine, fine!

Tom also suggested to align implementations as much as possible, and I do 
agree with him.

Also, I'd suggest to add a comment to explain that the precise C99 modulo 
semantics is required to make the algorithm work, and that it may not work 
with C89 semantics for instance.

>> Note: in the numeric code you abs the value, ISTM consistent to do it
>> as well in the other implementations.
>
> As noted in the comments, numeric does not have the INT_MIN problem.

Sure, but there are special cases at the beginning all the same: NAN, 
INTEGRAL…

>> Would it make sense that NAN is returned on NUMERIC when the 
>> computation cannot be performed, eg on non integer values?
>
> I don't think so, no.

Ok. Why? I do not have an opinion, but ISTM that there is a choice and it 
should be explained. Could be consistency with other cases, whatever.

>> Why the positive constraint on LCM(NUMERIC, NUMERIC)? Why not absoluting?
>
> I didn't see a definition for negative inputs, but now I see one so I've
> lifted the restriction.

Good.

About tests: again, I'd check the LCM overflow on smaller values.

I'm not convinced by the handling of fractional numerics in gcd, eg:

  +SELECT gcd(330.3::numeric, 462::numeric);
  + gcd
  +-----
  + 0.3
  +(1 row)

ISTM that the average user, including myself, would expect an integer 
result from gcd.

If this is kept, the documentation should be clear about what it does and 
what it means, because the least to say is that it is surprising.

Somehow I could have expected the arguments to be casted to int, so that 
it would lead to 66.

Python does a type error, which I find even better. I'd vote for erroring.

If this fractional gcd makes some sense and is desirable, then ISTM that 
lcm(a,b) = a / gcd(a, b) * b should make as much sense and should be 
allowed as well, for consistency.

-- 
Fabien.