> 0001: > > - You removed the check of parallelity check of > line_interpt_line(old line_interpt_internal) but line_parallel > is not changed so the consistency between the two functions are > lost. It is better to be another patch file (maybe 0004?).
I am making line_parallel() use line_interpt_line(). What they do is exactly the same. > - + Assert(p1->npts > 0 && p2->npts > 0); > > Other path_ functions are allowing path with no points so just > returning false if (p1->npts == 0 || p2->npts == 0) seems > enough. Assertion should be used only for something server > cannot continue working. (division by zero doesn't crash > server) I saw other similar assertions in the patch. path_in() and path_recv() reject paths with no points. I thought we shouldn't spend cycles for things that cannot happen. I can revert this part if you find previous practice better. > 0003: > > close_pl/ps/lseg/pb/ls/sb have changed to return null when > lseg_closept_line returns NAN, but they are defined as strict > and that is reasonable. Anyway pg_hypot returns NaN only when > parameters contain INF or NAN so we should error out when it > returns nan. I though strict is only related to the input being NULL. Tom Lane made close_ps() return NULL with commit 278148907a971ec7fa4ffb24248103d8012155d2. The other functions currently fail with elog()s from DirectFunctionCalls. I don't have strong preference for NULL or an error. Could you please suggest an errcode and errmsg, if you have? > circle_in rejects negative radius and circle_recv modived to > reject zero and negative. What is the reason for the change? Overlooking. Thank you for noticing. I am fixing it. > I understand that we don't need to consider NAN is equal to NAN. > circle_same, point_eq_point, line_eq no longer needs such > change? I though it would be better to consider NaNs equal only on the equality operators. That is why only they are changed that way. > Assertion is added in pg_hypot but it is impossible for result > to be negative there. Why do you added it? True. I am removing it. > 0004: > > Looking line_recv's change, I became to think that FPxx macros > is provided for coordinate values. I don't think it is applied > to non-coordinate values like coefficients. If some kind of > tolerance is needed here, it is not the same with the EPSILON. > > + if (FPzero(line->A) && FPzero(line->B)) > + ereport(ERROR, > > So, the above should be exact comparison with 0.0 and line_in > also should be the same. And maybe the same thing should be done > at many places.. I agree you. EPSILON is currently applied prematurely. I am trying to stay away from EPSION related issues to improve the chances to get this part committed. > But the following line of line_parallel still requires some kind > of tolerance to work properly. Since this patch is an > imoprovement patch, I think we can change it to the vector method. I am making line_parallel() use line_interpt_line() in response to your first remark. Vector based algorithm is probably better for every use of line_interpt_line(), but it is a bigger change with more user visible effects. > The problem of line_distance is an existing one so we can leave > it alone. It returns 0.0 for the most cases but it is a > long-standing behavior.. (Anyway I don't find a reasonable > definition of the distance between very-nearly parallel lines..) Exact comparison with 0.0 instead of FPzero() would also be an improvement for line_distance(), but I am not doing it now. > -- Sorry time's up today. I am waiting for the rest of your review to post the new versions.
