pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  

help / color / mirror / Atom feed
From: Fabien COELHO <coelho@cri.ensmp.fr>
To: Vik Fearing <vik.fearing@2ndquadrant.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Stephen Frost <sfrost@snowman.net>
Cc: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: Chapman Flack <chap@anastigmatix.net>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Greatest Common Divisor
Date: Sat, 4 Jan 2020 10:34:10 +0100 (CET)
Message-ID: <alpine.DEB.2.21.2001041003090.31333@pseudo> (raw)
In-Reply-To: <32b8af34-8f87-c30f-5555-2325bd140a59@2ndquadrant.com>
References: <adbd3e0b-e3f1-5bbc-21db-03caf1cef0f7@2ndquadrant.com>
	<alpine.DEB.2.21.1912281848540.24861@pseudo>
	<dbf86808-91fa-3262-f939-2ef3d38ec242@2ndquadrant.com>
	<alpine.DEB.2.21.1912290815520.889@pseudo>
	<5E08D947.9000902@anastigmatix.net>
	<alpine.DEB.2.21.1912291755510.14206@pseudo>
	<CAEZATCX9YgkWupVFcJ5ixkEJMyf2X5ZecN8K5r12oOB8mDHzWQ@mail.gmail.com>
	<20200102145712.GN3195@tamriel.snowman.net>
	<16242.1577977961@sss.pgh.pa.us>
	<6855727c-3836-21cc-18f9-e79227b99079@2ndquadrant.com>
	<alpine.DEB.2.21.2001032010470.12030@pseudo>
	<32b8af34-8f87-c30f-5555-2325bd140a59@2ndquadrant.com>


Bonjour Vik,

> Here is an updated patch fixing that.

As I said, welcome to arithmetic:-)

Patch v5 applies cleanly.

Doc: I'd consider using an example the result of which is 42 instead of 
21, for obvious geek motivations. Possibly gcd(2142, 462) should be ok.

I got it wrong with my previous comment on gcd(int_min, 0). I'm okay with 
erroring on int_min.

Code comments: gcd(n, 0) = abs(n), not n?

About the code.

Add unlikely() where appropriate.

I'd deal with int_min and 0 at the beginning and then proceed with 
absoluting the values, rather than the dance around a1/arg1, a2/arg2, and 
other arg2 = -arg2, and arg1 = -arg1 anyway in various places, which does 
not make the code that easy to understand.

Pseudo code could be:

    if ((a1 == min && (a2 == min || a2 == 0)) ||
        (a2 == min && a1 == 0))
      error;
    a1 = abs(a1), a2 = abs(a2);
    euclids;
    return;

Note: in the numeric code you abs the value, ISTM consistent to do it as 
well in the other implementations.

Would it make sense that NAN is returned on NUMERIC when the computation 
cannot be performed, eg on non integer values?

Why the positive constraint on LCM(NUMERIC, NUMERIC)? Why not absoluting?

Tests: you can make LCM fail on much smaller values for int2/4/8, you do 
not need to start around max_int.

-- 
Fabien.





view thread (70+ messages)  latest in thread

Message-ID: <alpine.DEB.2.21.2001041003090.31333@pseudo>
Permalink:  ../alpine.DEB.2.21.2001041003090.31333@pseudo/
Also on:    postgresql.org/message-id/alpine.DEB.2.21.2001041003090.31333@pseudo

 · 

reply
Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-hackers@postgresql.org
  Cc: coelho@cri.ensmp.fr, vik.fearing@2ndquadrant.com, tgl@sss.pgh.pa.us, sfrost@snowman.net, dean.a.rasheed@gmail.com, chap@anastigmatix.net, pgsql-hackers@lists.postgresql.org
  Subject: Re: Greatest Common Divisor
  In-Reply-To: <alpine.DEB.2.21.2001041003090.31333@pseudo>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox