Skip to content

Commit

Permalink
Avoid FE_INVALID on operations with empty Envelopes
Browse files Browse the repository at this point in the history
  • Loading branch information
@dbaston
dbaston committed Jan 10, 2023
1 parent a8514a3 commit 30550d2
Show file tree
Hide file tree
Showing 5 changed files with 117 additions and 31 deletions.
41 changes: 22 additions & 19 deletions include/geos/geom/Envelope.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -454,16 +454,16 @@ class GEOS_DLL Envelope {
maxy = other->maxy;
}
else {
if(other->minx < minx) {
if(std::isless(other->minx, minx)) {
minx = other->minx;
}
if(other->maxx > maxx) {
if(std::isgreater(other->maxx, maxx)) {
maxx = other->maxx;
}
if(other->miny < miny) {
if(std::isless(other->miny, miny)) {
miny = other->miny;
}
if(other->maxy > maxy) {
if(std::isgreater(other->maxy, maxy)) {
maxy = other->maxy;
}
}
Expand Down Expand Up @@ -535,8 +535,8 @@ class GEOS_DLL Envelope {
*/
bool intersects(const CoordinateXY& other) const
{
return (other.x <= maxx && other.x >= minx &&
other.y <= maxy && other.y >= miny);
return (std::islessequal(other.x, maxx) && std::isgreaterequal(other.x, minx) &&
std::islessequal(other.y, maxy) && std::isgreaterequal(other.y, miny));
}

/** \brief
Expand All @@ -548,7 +548,10 @@ class GEOS_DLL Envelope {
*/
bool intersects(double x, double y) const
{
return (x <= maxx && x >= minx && y <= maxy && y >= miny);
return std::islessequal(x, maxx) &&
std::isgreaterequal(x, minx) &&
std::islessequal(y, maxy) &&
std::isgreaterequal(y, miny);
}

/** \brief
Expand All @@ -559,10 +562,10 @@ class GEOS_DLL Envelope {
*/
bool intersects(const Envelope* other) const
{
return other->minx <= maxx &&
other->maxx >= minx &&
other->miny <= maxy &&
other->maxy >= miny;
return std::islessequal(other->minx, maxx) &&
std::isgreaterequal(other->maxx, minx) &&
std::islessequal(other->miny, maxy) &&
std::isgreaterequal(other->maxy, miny);
}

bool intersects(const Envelope& other) const
Expand All @@ -584,10 +587,10 @@ class GEOS_DLL Envelope {

bool disjoint(const Envelope* other) const
{
return !(other->minx <= maxx ||
other->maxx >= minx ||
other->miny <= maxy ||
other->maxy >= miny);
return !(std::islessequal(other->minx, maxx) ||
std::isgreaterequal(other->maxx, minx) ||
std::islessequal(other->miny, maxy) ||
std::isgreaterequal(other->maxy, miny));
}

/** \brief
Expand All @@ -598,10 +601,10 @@ class GEOS_DLL Envelope {
* @return `true` if `(x, y)` lies in the interior or on the boundary of this Envelope.
*/
bool covers(double x, double y) const {
return x >= minx &&
x <= maxx &&
y >= miny &&
y <= maxy;
return std::isgreaterequal(x, minx) &&
std::islessequal(x, maxx) &&
std::isgreaterequal(y, miny) &&
std::islessequal(y, maxy);
}

/** \brief
Expand Down
18 changes: 9 additions & 9 deletions src/geom/Envelope.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,22 +58,22 @@ Envelope::intersects(const CoordinateXY& a, const CoordinateXY& b) const
// std::minmax performs no better and compiles down to more
// instructions.
double envminx = (a.x < b.x) ? a.x : b.x;
if(!(maxx >= envminx)) { // awkward comparison catches cases where this->isNull()
if(!std::isgreaterequal(maxx, envminx)) { // awkward comparison catches cases where this->isNull()
return false;
}

double envmaxx = (a.x > b.x) ? a.x : b.x;
if(envmaxx < minx) {
if(std::isless(envmaxx, minx)) {
return false;
}

double envminy = (a.y < b.y) ? a.y : b.y;
if(envminy > maxy) {
if(std::isgreater(envminy, maxy)) {
return false;
}

double envmaxy = (a.y > b.y) ? a.y : b.y;
if(envmaxy < miny) {
if(std::isless(envmaxy, miny)) {
return false;
}

Expand Down Expand Up @@ -105,10 +105,10 @@ bool
Envelope::covers(const Envelope& other) const
{
return
other.minx >= minx &&
other.maxx <= maxx &&
other.miny >= miny &&
other.maxy <= maxy;
std::isgreaterequal(other.minx, minx) &&
std::islessequal(other.maxx, maxx) &&
std::isgreaterequal(other.miny, miny) &&
std::islessequal(other.maxy, maxy);
}

/*public*/
Expand Down Expand Up @@ -233,7 +233,7 @@ Envelope::expandBy(double deltaX, double deltaY)
maxy += deltaY;

// check for envelope disappearing
if(minx > maxx || miny > maxy) {
if(std::isgreater(minx, maxx) || std::isgreater(miny, maxy)) {
setToNull();
}
}
Expand Down
14 changes: 14 additions & 0 deletions tests/unit/capi/GEOSIntersectionTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,20 @@ void object::test<6>
// No memory leaked
}

// https://github.com/libgeos/geos/pull/790
template<>
template<>
void object::test<7>
()
{
geom1_ = GEOSGeomFromWKT("POLYGON ((1 0, 1 1, 0 1, 0 0, 1 0))");
geom2_ = GEOSGeomFromWKT("POLYGON ((1 2, 1 3, 0 3, 0 2, 1 2))");

result_ = GEOSIntersection(geom1_, geom2_);

ensure(!std::fetestexcept(FE_INVALID));
}


} // namespace tut

3 changes: 3 additions & 0 deletions tests/unit/capi/capi_test_utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <cstdio>
#include <cstdlib>
#include <cstring>
#include <cfenv>


namespace capitest {
Expand All @@ -29,6 +30,8 @@ namespace capitest {
wktw_ = GEOSWKTWriter_create();
GEOSWKTWriter_setTrim(wktw_, 1);
GEOSWKTWriter_setRoundingPrecision(wktw_, 10);

std::feclearexcept(FE_ALL_EXCEPT);
}

~utility()
Expand Down
72 changes: 69 additions & 3 deletions tests/unit/geom/EnvelopeTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,31 @@
#include <geos/geom/Envelope.h>
#include <geos/geom/Coordinate.h>

#include <cfenv>

namespace tut {
//
// Test Group
//

using geos::geom::Envelope;

// dummy data, not used
struct test_envelope_data {};
struct test_envelope_data {
test_envelope_data()
{
std::feclearexcept(FE_ALL_EXCEPT);
}

static void ensure_no_fp_except()
{
ensure("FE_DIVBYZERO raised", !std::fetestexcept(FE_DIVBYZERO));
//ensure("FE_INEXACT raised", !std::fetestexcept(FE_INEXACT));
ensure("FE_INVALID raised", !std::fetestexcept(FE_INVALID));
ensure("FE_OVERFLOW raised", !std::fetestexcept(FE_OVERFLOW));
ensure("FE_UNDERFLOW raised", !std::fetestexcept(FE_UNDERFLOW));
}
};

typedef test_group<test_envelope_data> group;
typedef group::object object;
Expand All @@ -36,6 +54,8 @@ void object::test<1>

ensure_equals(empty.getWidth(), 0);
ensure_equals(empty.getHeight(), 0);

ensure_no_fp_except();
}

// 2 - Test of overridden constructor
Expand All @@ -57,6 +77,8 @@ void object::test<2>
ensure_equals(box.getMaxX(), box.getMaxY());

ensure_equals(box.getWidth(), box.getHeight());

ensure_no_fp_except();
}

// 3 - Test of copy constructor
Expand All @@ -75,6 +97,8 @@ void object::test<3>
ensure(!copied.isNull());
ensure(copied == box);
ensure_equals(copied.getWidth(), copied.getHeight());

ensure_no_fp_except();
}

// 4 - Test of setToNull()
Expand All @@ -88,6 +112,8 @@ void object::test<4>
ensure(!e.isNull());
e.setToNull();
ensure(e.isNull());

ensure_no_fp_except();
}

// 5 - Test of equals()
Expand Down Expand Up @@ -117,6 +143,8 @@ void object::test<5>

ensure(!box.equals(&empty));
ensure(!box.equals(&zero));

ensure_no_fp_except();
}

// 6 - Test of contains()
Expand Down Expand Up @@ -160,6 +188,8 @@ void object::test<6>
ensure_equals(origin.y, 0);
ensure_equals(origin.z, 0);
ensure(small.contains(origin));

ensure_no_fp_except();
}

// Test of intersects() and disjoint()
Expand Down Expand Up @@ -213,6 +243,8 @@ void object::test<7>
ensure(with_origin.intersects(origin));

ensure("empty envelope does not intersect coordinate", !empty.intersects(origin));

ensure_no_fp_except();
}

// Test of expand()
Expand All @@ -238,6 +270,8 @@ void object::test<8>
empty.expandToInclude(&box);
ensure("expanding null envelope to include non-null envelope makes null envelope not null",
empty == exemplar);

ensure_no_fp_except();
}

// Second test of expand()
Expand All @@ -257,6 +291,8 @@ void object::test<9>
// Expand empty envelope to include bigger envelope
empty.expandToInclude(&box);
ensure(empty == exemplar);

ensure_no_fp_except();
}

// Test point-to-envelope distance
Expand All @@ -278,11 +314,9 @@ void object::test<10>
// 20 21 22 23 24
std::vector<Coordinate> c(25);

// std::cout<<std::endl;
for (std::size_t i = 0; i < c.size(); i++) {
c[i].x = static_cast<double>(i % 5);
c[i].y = static_cast<double>(5 - (i / 5));
// std::cout<< c[i] << std::endl;
}

// point contained in envelope
Expand Down Expand Up @@ -319,6 +353,7 @@ void object::test<10>
ensure_equals(Envelope::distanceToCoordinate(c[6], c[12], c[14]),
c[6].distance(c[12]));

ensure_no_fp_except();
}

// Test envelope distance
Expand Down Expand Up @@ -371,7 +406,38 @@ void object::test<11>
b = Envelope({17, 3}, {20, 5});
ensure_equals(a.distance(b), Coordinate(13, 11).distance(Coordinate(17, 5)));
ensure_equals(a.distance(b), b.distance(a));

ensure_no_fp_except();
}

// comparison of empty envelopes
template<>
template<>
void object::test<12>
()
{
Envelope empty1;
Envelope empty2;

ensure(!(empty1 < empty2));
ensure(!(empty2 < empty1));

ensure_no_fp_except();
}

// Envelope::intersects(Coordinate, Coordinate)
template<>
template<>
void object::test<13>
()
{
Envelope empty;

ensure(!empty.intersects({1, 1}, {2, 2}));

ensure_no_fp_except();
}


} // namespace tut

0 comments on commit 30550d2

Please sign in to comment.