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 <e...@hasegeli.com> wrote in <CAE2gYzz2=335XEMHK-neNo=hggskkphqxuxkh8ydnszancb...@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 <e...@hasegeli.com> wrote in <cae2gyzz1kut57vro-xzk3ksqdpehaupjplyu7ashuuwrf7m...@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.7070...@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/407d949e0908222139t35ad3ad2q3e6b15646a27d...@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 (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers