Re: [HACKERS] [PATCH]: fix bug in SP-GiST box_ops

Hello,

On Thu, March 9, 2017 9:04 am, Alexander Korotkov wrote:
> On Wed, Feb 1, 2017 at 2:43 PM, Emre Hasegeli <emre@hasegeli.com> wrote:
>
>> > I think this patch is already in a good shape.
>>
>> I am sorry for introducing this bug.  This fix looks good to me as well.
>
>
> I checked this patch too.  And it seems good to me as well.
> Should we mark it as "ready for committer"?

I can't comment on the code, but the grammar on the comments caught my eye:

> +/* Can any range from range_box does not extend higher than this
argument? */
>
>+static bool
>+overLower2D(RangeBox *range_box, Range *query)
>+{
>+    return FPle(range_box->left.low, query->high) &&
>+        FPle(range_box->right.low, query->high);
>+}

The sentence sounds quite garbled in English. I'm not entirely sure what
it should be, but given the comment below "/* Can any range from range_box
to be higher than this argument? */" maybe something like:

/* Does any range from range_box extend to the right side of the query? */

If used, an analog wording should be used for overHigher2D's comment like:

/* Does any range from range_box extend to the left side of the query? */

Also:

/* Can any range from range_box to be higher than this argument? */

should be:

/* Can any range from range_box be higher than this argument? */

Another question: Does it make sense to add the "minimal bad example for
the '&<' case" as test case, too? After all, it should pass the test after
the patch.

Bets regards,

Tels