Re: [HACKERS] [PATCH] Improve geometric types - Mailing list pgsql-hackers

Hello. Thank you for the new version.

0001: applies cleanly. regress passed.     this mainly refactoring geo_ops.c and replacing pg_hypot with hypot(3).
0002: applies cleanly. regress passed.     this just replaces float-ops macros into inline functions.
0003: applies cleanly. regress passed.     replaces double with float8 and bare arithmetic with defined functions.
0004: applies cleanly. regress passsed.     fix broken line-related functions.     I have some comments on this
(later).

At Wed, 27 Sep 2017 17:44:52 +0200, Emre Hasegeli <emre@hasegeli.com> wrote in
<CAE2gYzz2=335XEMHK-neNo=HgGskkPHQxUXkh8yDNsZAnCB5Bg@mail.gmail.com>
> The new versions of the patches are attached addressing your comments.
> 
> > C++ surely make just static functions inlined but I'm not sure C
> > compiler does that.
> 
> Thank you for your explanation.  I marked the mentioned functions "inline".

Thanks.

> > So we should be safe to have a buffer with 26 byte length and 500
> > bytes will apparently too large and even 128 will be too loose in
> > most cases. So how about something like the following?
> >
> > #define MINDOUBLEWIDTH 32
> 
> I left this part out for now.  We can improve it separately.

Agreed. I found that psprintf does that with initial length of
128.

By the way, I found that MAXDOUBLEWIDTH has two inconsistent
definitions in formatting.c(500) and float.c(128). The definition
in new float.h is according to float.c and it seems better to be
untouched and it would be another issue.


At Wed, 27 Sep 2017 17:45:04 +0200, Emre Hasegeli <emre@hasegeli.com> wrote in
<CAE2gYzz1KuT57vrO-XZk3KSqdpehAupJPLYu7AshuUwRF7MiMg@mail.gmail.com>
> > The patch replace pg_hypot with hypot in libc. The man page says
> > as follows.
...
> I included them on the latest version.

# The commit message of 0001 says that "using C11 hypot()" but it
# is sine C99. I suppose we shold follow C99 at the time so I
# suggest rewrite it in the next version if any.

It seems bettern than expected.  I confirmed that
pg_hypot(DBL_MAX, DBL_MAX) returned a value that is equivalent to
HUGE_VAL * HUGE_VAL on glibc, but I'm not sure the behavior on
other platforms is the same.

======
For other potential reviewers:

I found the origin of the function here.

https://www.postgresql.org/message-id/4A90BD76.7070804@netspace.net.au
https://www.postgresql.org/message-id/AANLkTim4cHELcGPf5w7Zd43_dQi_2RJ_b5_F_idSSbZI%40mail.gmail.com

And the reason for pg_hypot is seen here.

https://www.postgresql.org/message-id/407d949e0908222139t35ad3ad2q3e6b15646a27dd64@mail.gmail.com

I think the replacement is now acceptable according to the discussion.
======

And this should be the last comment of mine on the patch set.

In 0004, line_distance and line_interpt_internal seem
correct. Seemingly horizontal/vertical checks are redundant but
it is because unclearly defined EPSLON bahavior. lseg_perp seems
correct.

close_pl got a bug fix not only refactoring. I think it is
recommended to separate bugs and improvements, but I'm fine with
the current patch.


You added sanity check "A==0 && B==0" (in Ax + By + C) in
line_recv. I'm not sure the necessity of the check since it has
been checked in line_in but anyway the comparisons seem to be
typo(or thinko) of FPzero.

dist_pl is changed to take the smaller distance of both ends of
the segment. It seems absorbing error, so it might be better
taking the mean of the two distances. If you have a firm reason
for the change, it is better to be written there, or it might be
better left alone.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers