From: Tom Lane <tgl@sss.pgh.pa.us>
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: emre@hasegeli.com, Ning Yu <nyu@pivotal.io>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: New Defects reported by Coverity Scan for PostgreSQL
Date: Wed, 01 Aug 2018 09:57:04 -0400
Message-ID: <26723.1533131824@sss.pgh.pa.us> (raw)
In-Reply-To: <5fece6c5-6a80-9694-1a24-3ae245ae2cba@2ndquadrant.com>
References: <E1fjZo1-00019i-GG@gemulon.postgresql.org>
<20180731172149.kdjl3xdjjtwxaj4s@alvherre.pgsql>
<CAKmaiL2o6584gLFP=6YX-MUqmvOiT==uny+LA8z3=uAnriuzXA@mail.gmail.com>
<26769.1533090136@sss.pgh.pa.us>
<CAE2gYzw6iD1YTZ+67hA=s3AG5sbLCsGkHRYQ_Z1Kd+vhiM7ecA@mail.gmail.com>
<5fece6c5-6a80-9694-1a24-3ae245ae2cba@2ndquadrant.com>
Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> On 08/01/2018 11:55 AM, Emre Hasegeli wrote:
>> Consistency. I organized all xxx_closept_yyy(Point *result, xxx *l1,
>> yyy *l2) functions in a way that they find the find the point on "l1".
> IMHO the main issue here is that the rule is not obvious / documented
> anywhere. I think the best way to do that is by making it clear in a
> comment for each such such function.
I think there are three different things that need to be addressed:
* Underspecified comments.
* The function names and argument names are badly chosen IMO, because even
granted a convention such as the above, it's not very obvious what roles
"l1" and "l2" play. I'm not exactly sure what would be better, but if you
used names like "ofseg" and "otherseg" you'd at least be trying. I'd go
with an asymmetrical function name too, to make miswriting of calls less
likely.
* And lastly, are we sure there aren't actual *bugs* here? I'd initially
supposed that lseg_closept_lseg acted as Emre says above, but reading the
code makes me think it's the other way around. Its first two potential
assignments to *result are definitely assigning points on l2 not l1.
regards, tom lane
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: tgl@sss.pgh.pa.us, tomas.vondra@2ndquadrant.com, nyu@pivotal.io, alvherre@2ndquadrant.com, pgsql-hackers@postgresql.org
Subject: Re: New Defects reported by Coverity Scan for PostgreSQL
In-Reply-To: <26723.1533131824@sss.pgh.pa.us>
* 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