Slaying the HYPOTamus

Like some ancient precursor to the modern hypot()enuse the dreaded
ill-tempered HYPOT()amus lurks in the recesses of geo_ops.c and
geo_decls.h. And many a line of code has been swallowed by its mighty maw.

This patch replaces the HYPOT() macro with a calls to the hypot() function.

The hypot() function has been part of the C standard since C99 (ie 10
years ago), and in most UNIX, IBM and GNU libraries longer than that.
The function is designed not to fail where the current naive macro would
result in overflow. Where available, we might expect to the hypot()
function to take advantage of underlying hardware opcodes. In addition
the function evaluates its arguments only once. The current macro
evaluates its arguments twice.

Currently
HYPOT(a.x - b.x, a.y - b.y))
becomes:
sqrt((a.x - b.x)*(a.x - b.x) + (a.y - b.y) * (a.y - b.y))

The patch passes all test.

Patch attached below. (First attempt at using CVS. Hope it's all good)

On Sun, Aug 23, 2009 at 4:54 AM, Paul Matthews<plm(at)netspace(dot)net(dot)au> wrote:
>
> The hypot() function has been part of the C standard since C99 (ie 10
> years ago)

Postgres targets C89. The date of the standard is when the standard
came out, it takes years before it's widely available and then years
again before the systems with the old compiler are no longer
interesting.

If there's a performance advantage then we could add a configure test
and define the macro to call hypot(). You said it existed before C99
though, how widespread was it? If it's in all the platforms we support
it might be reasonable to just go with it.

> The function is designed not to fail where the current naive macro would
> result in overflow.

The code seems to be blissfully unaware of overflow dangers :( Even if
hypot() avoids spurious overflows it's always possible for there to be
a legitimate overflow which we ought to detect and handle properly.

--
greg
http://mit.edu/~gsstark/resume.pdf

Greg Stark <gsstark(at)mit(dot)edu> writes:
> If there's a performance advantage then we could add a configure test
> and define the macro to call hypot(). You said it existed before C99
> though, how widespread was it? If it's in all the platforms we support
> it might be reasonable to just go with it.

For one data point, I see hypot() in HPUX 10.20, released circa 1996.
I suspect we would want a configure test and a substitute function
anyway. Personally I wouldn't have a problem with the substitute being
the naive sqrt(x*x+y*y), particularly if it's replacing existing code
that overflows in the same places.

regards, tom lane

On Sun, Aug 23, 2009 at 07:42, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Greg Stark <gsstark(at)mit(dot)edu> writes:
>> If there's a performance advantage then we could add a configure test
>> and define the macro to call hypot(). You said it existed before C99
>> though, how widespread was it? If it's in all the platforms we support
>> it might be reasonable to just go with it.
>
> For one data point, I see hypot() in HPUX 10.20, released circa 1996.
> I suspect we would want a configure test and a substitute function
> anyway.  Personally I wouldn't have a problem with the substitute being
> the naive sqrt(x*x+y*y), particularly if it's replacing existing code
> that overflows in the same places.

For another data point, Microsoft documentation says:
"This POSIX function is deprecated beginning in Visual C++ 2005. Use
the ISO C++ conformant _hypot instead."

_hypot() has been there since Windows 95, so it shouldn't be a problem
to use it - it just needs a define, like we have for some other such
functions.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

2009/8/23 Magnus Hagander <magnus(at)hagander(dot)net>:

> For another data point, Microsoft documentation says:
> "This POSIX function is deprecated beginning in Visual C++ 2005. Use
> the ISO C++ conformant  _hypot instead."
>
> _hypot() has been there since Windows 95, so it shouldn't be a problem
> to use it - it just needs a define, like we have for some other such
> functions.

FYI, according to [1], this warning is bogus (i.e., "hypot" without
underscore should be entirely OK) and will be removed in VS 2010.

Nicolas

[1] <url:http://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx?FeedbackID=289653>