From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Emre Hasegeli <emre@hasegeli.com>
Cc: tgl@sss.pgh.pa.us
Cc: tomas.vondra@2ndquadrant.com
Cc: nyu@pivotal.io
Cc: pgsql-hackers@postgresql.org
Subject: Re: New Defects reported by Coverity Scan for PostgreSQL
Date: Tue, 6 Nov 2018 14:32:10 -0300
Message-ID: <20181106173210.46xq6mcsp3b6a2h7@alvherre.pgsql> (raw)
In-Reply-To: <CAE2gYzx2BgYm=M9Cx0e1cCPPyA7ecWry_0=8oGfCcFEU=KTJUA@mail.gmail.com>
On 2018-Nov-06, Emre Hasegeli wrote:
> The patch is attached to improve the comments and variable names. I
> explained the functions with the same signature on the file header. I
> can duplicate those on the function headers if you find that better.
Surely the comment in line 3839 deserves an update :-)
This seems good material. I would put the detailed conventions comment
separately from the head of the file, like this (where I also changed
"Type1 *type1" into "Type1 *obj1", and a few "has" to "have")
Thanks
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:geo-ops-comments-v01.patch (text/x-diff, 9.8K) downloaddiff --git a/src/backend/utils/adt/geo_ops.c b/src/backend/utils/adt/geo_ops.cindex e7c1160131..620d8a8c80 100644--- a/src/backend/utils/adt/geo_ops.c+++ b/src/backend/utils/adt/geo_ops.c@@ -3,6 +3,16 @@
* geo_ops.c
* 2D geometric operations
*
+ * This module implements the geometric functions and operators. The+ * geometric types are (from simple to more complicated):+ *+ * - point+ * - line+ * - line segment+ * - box+ * - circle+ * - polygon+ *
* Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
@@ -25,6 +35,39 @@
#include "utils/fmgrprotos.h"
#include "utils/geo_decls.h"
+/*+ * The functions to implement the types have the signature:+ *+ * void type_construct(Type *result, ...);+ *+ * The functions to implement operators usually have signatures like:+ *+ * void type1_operator_type2(Type *result, Type1 *obj1, Type2 *obj2);+ *+ * There are certain operators between different types. The functions+ * that return the intersection point between 2 types has signature:+ *+ * bool type1_interpt_type2(Point *result, Type1 *obj1, Type2 *obj2);+ *+ * These return whether the two objects intersect, and set the intersection+ * point to pre-allocated *result when it is not NULL. Those functions may be+ * used to implement multiple SQL level operators. For example, determining+ * whether two lines are parallel is done by checking whether they intersect.+ *+ * The functions that return whether an object contains another have+ * the signature:+ *+ * bool type1_contain_type2(Type1, *obj1, Type2 *obj2);+ *+ * The functions to get the closest point on an object to the second object+ * have the signature:+ *+ * float8 type1_closept_type2(Point *result, Type1 *obj1, Type2 *obj2);+ *+ * They return the shortest distance between the two objects, and set+ * the closest point on the first object to the second object to pre-allocated+ * *result when it is not NULL.+ */
/*
* Internal routines
@@ -64,7 +107,7 @@ static int lseg_crossing(float8 x, float8 y, float8 px, float8 py);
static bool lseg_contain_point(LSEG *lseg, Point *point);
static float8 lseg_closept_point(Point *result, LSEG *lseg, Point *pt);
static float8 lseg_closept_line(Point *result, LSEG *lseg, LINE *line);
-static float8 lseg_closept_lseg(Point *result, LSEG *l1, LSEG *l2);+static float8 lseg_closept_lseg(Point *result, LSEG *on_lseg, LSEG *to_lseg);
/* Routines for boxes */
static inline void box_construct(BOX *result, Point *pt1, Point *pt2);
@@ -74,7 +117,7 @@ static float8 box_ar(BOX *box);
static float8 box_ht(BOX *box);
static float8 box_wd(BOX *box);
static bool box_contain_point(BOX *box, Point *point);
-static bool box_contain_box(BOX *box1, BOX *box2);+static bool box_contain_box(BOX *contains_box1, BOX *contained_box2);
static bool box_contain_lseg(BOX *box, LSEG *lseg);
static bool box_interpt_lseg(Point *result, BOX *box, LSEG *lseg);
static float8 box_closept_point(Point *result, BOX *box, Point *point);
@@ -87,7 +130,7 @@ static float8 circle_ar(CIRCLE *circle);
static void make_bound_box(POLYGON *poly);
static void poly_to_circle(CIRCLE *result, POLYGON *poly);
static bool lseg_inside_poly(Point *a, Point *b, POLYGON *poly, int start);
-static bool poly_contain_poly(POLYGON *polya, POLYGON *polyb);+static bool poly_contain_poly(POLYGON *contains_poly, POLYGON *contained_poly);
static bool plist_same(int npts, Point *p1, Point *p2);
static float8 dist_ppoly_internal(Point *pt, POLYGON *poly);
@@ -651,12 +694,12 @@ box_contain(PG_FUNCTION_ARGS)
* Check whether the box is in the box or on its border
*/
static bool
-box_contain_box(BOX *box1, BOX *box2)+box_contain_box(BOX *contains_box, BOX *contained_box)
{
- return FPge(box1->high.x, box2->high.x) &&- FPle(box1->low.x, box2->low.x) &&- FPge(box1->high.y, box2->high.y) &&- FPle(box1->low.y, box2->low.y);+ return FPge(contains_box->high.x, contained_box->high.x) &&+ FPle(contains_box->low.x, contained_box->low.x) &&+ FPge(contains_box->high.y, contained_box->high.y) &&+ FPle(contains_box->low.y, contained_box->low.y);
}
@@ -1223,10 +1266,6 @@ line_interpt(PG_FUNCTION_ARGS)
/*
* Internal version of line_interpt
*
- * This returns true if two lines intersect (they do, if they are not- * parallel), false if they do not. This also sets the intersection point- * to *result, if it is not NULL.- *
* NOTE: If the lines are identical then we will find they are parallel
* and report "no intersection". This is a little weird, but since
* there's no *unique* intersection, maybe it's appropriate behavior.
@@ -2246,8 +2285,6 @@ lseg_center(PG_FUNCTION_ARGS)
/*
* Find the intersection point of two segments (if any).
*
- * This returns true if two line segments intersect, false if they do not.- * This also sets the intersection point to *result, if it is not NULL.
* This function is almost perfectly symmetric, even though it doesn't look
* like it. See lseg_interpt_line() for the other half of it.
*/
@@ -2508,10 +2545,6 @@ dist_ppoly_internal(Point *pt, POLYGON *poly)
/*
* Check if the line segment intersects with the line
- *- * This returns true if line segment intersects with line, false if they- * do not. This also sets the intersection point to *result, if it is not- * NULL.
*/
static bool
lseg_interpt_line(Point *result, LSEG *lseg, LINE *line)
@@ -2561,9 +2594,6 @@ lseg_interpt_line(Point *result, LSEG *lseg, LINE *line)
/*
* The intersection point of a perpendicular of the line
* through the point.
- *- * This sets the closest point to the *result if it is not NULL and returns- * the distance to the closest point.
*/
static float8
line_closept_point(Point *result, LINE *line, Point *point)
@@ -2609,9 +2639,6 @@ close_pl(PG_FUNCTION_ARGS)
/*
* Closest point on line segment to specified point.
- *- * This sets the closest point to the *result if it is not NULL and returns- * the distance to the closest point.
*/
static float8
lseg_closept_point(Point *result, LSEG *lseg, Point *pt)
@@ -2650,27 +2677,24 @@ close_ps(PG_FUNCTION_ARGS)
/*
* Closest point on line segment to line segment
- *- * This sets the closest point to the *result if it is not NULL and returns- * the distance to the closest point.
*/
static float8
-lseg_closept_lseg(Point *result, LSEG *l1, LSEG *l2)+lseg_closept_lseg(Point *result, LSEG *on_lseg, LSEG *to_lseg))
{
Point point;
float8 dist,
d;
/* First, we handle the case when the line segments are intersecting. */
- if (lseg_interpt_lseg(result, l1, l2))+ if (lseg_interpt_lseg(result, on_lseg, to_lseg))
return 0.0;
/*
* Then, we find the closest points from the endpoints of the second
* line segment, and keep the closest one.
*/
- dist = lseg_closept_point(result, l1, &l2->p[0]);- d = lseg_closept_point(&point, l1, &l2->p[1]);+ dist = lseg_closept_point(result, on_lseg, &to_lseg->p[0]);+ d = lseg_closept_point(&point, on_lseg, &to_lseg->p[1]);
if (float8_lt(d, dist))
{
dist = d;
@@ -2679,19 +2703,19 @@ lseg_closept_lseg(Point *result, LSEG *l1, LSEG *l2)
}
/* The closest point can still be one of the endpoints, so we test them. */
- d = lseg_closept_point(NULL, l2, &l1->p[0]);+ d = lseg_closept_point(NULL, to_lseg, &on_lseg->p[0]);
if (float8_lt(d, dist))
{
dist = d;
if (result != NULL)
- *result = l1->p[0];+ *result = on_lseg->p[0];
}
- d = lseg_closept_point(NULL, l2, &l1->p[1]);+ d = lseg_closept_point(NULL, to_lseg, &on_lseg->p[1]);
if (float8_lt(d, dist))
{
dist = d;
if (result != NULL)
- *result = l1->p[1];+ *result = on_lseg->p[1];
}
return dist;
@@ -2718,9 +2742,6 @@ close_lseg(PG_FUNCTION_ARGS)
/*
* Closest point on or in box to specified point.
- *- * This sets the closest point to the *result if it is not NULL and returns- * the distance to the closest point.
*/
static float8
box_closept_point(Point *result, BOX *box, Point *pt)
@@ -2837,9 +2858,6 @@ close_sl(PG_FUNCTION_ARGS)
/*
* Closest point on line segment to line.
*
- * This sets the closest point to the *result if it is not NULL and returns- * the distance to the closest point.- *
* NOTE: When the lines are parallel, endpoints of one of the line segment
* are FPeq(), in presence of NaN or Infinitive coordinates, or perhaps =
* even because of simple roundoff issues, there may not be a single closest
@@ -2895,9 +2913,6 @@ close_ls(PG_FUNCTION_ARGS)
/*
* Closest point on or in box to line segment.
- *- * This sets the closest point to the *result if it is not NULL and returns- * the distance to the closest point.
*/
static float8
box_closept_lseg(Point *result, BOX *box, LSEG *lseg)
@@ -3825,25 +3840,25 @@ lseg_inside_poly(Point *a, Point *b, POLYGON *poly, int start)
* Determine if polygon A contains polygon B.
*-----------------------------------------------------------------*/
static bool
-poly_contain_poly(POLYGON *polya, POLYGON *polyb)+poly_contain_poly(POLYGON *contains_poly, POLYGON *contained_poly)
{
int i;
LSEG s;
- Assert(polya->npts > 0 && polyb->npts > 0);+ Assert(contains_poly->npts > 0 && contained_poly->npts > 0);
/*
- * Quick check to see if bounding box is contained.+ * Quick check to see if bounding box is contained_poly.
*/
- if (!box_contain_box(&polya->boundbox, &polyb->boundbox))+ if (!box_contain_box(&contains_poly->boundbox, &contained_poly->boundbox))
return false;
- s.p[0] = polyb->p[polyb->npts - 1];+ s.p[0] = contained_poly->p[contained_poly->npts - 1];- for (i = 0; i < polyb->npts; i++)+ for (i = 0; i < contained_poly->npts; i++)
{
- s.p[1] = polyb->p[i];- if (!lseg_inside_poly(s.p, s.p + 1, polya, 0))+ s.p[1] = contained_poly->p[i];+ if (!lseg_inside_poly(s.p, s.p + 1, contains_poly, 0))
return false;
s.p[0] = s.p[1];
}
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-committers@postgresql.org
Cc: alvherre@2ndquadrant.com, emre@hasegeli.com, tgl@sss.pgh.pa.us, tomas.vondra@2ndquadrant.com, nyu@pivotal.io, pgsql-hackers@postgresql.org
Subject: Re: New Defects reported by Coverity Scan for PostgreSQL
In-Reply-To: <20181106173210.46xq6mcsp3b6a2h7@alvherre.pgsql>
* 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