Possible bug

[mloskot]

I'm not sure but it looks like a bug. The first formal parameter of Octant::octant static method is unused, instead second parameter is passed twice:

Here is code at Octant.h:63​

/**
 * Returns the octant of a directed line segment from p0 to p1.
 */
static int octant(const geom::Coordinate* p0, const geom::Coordinate* p1)
{
   return octant(*p1, *p1);
}

Even if it is not a bug, I believe it's nothing wrong to double-check.

[denisem]

The fix, on line 63:

[denise@localhost noding]$ diff -p Octant.h.orig Octant.h
*** Octant.h.orig       2007-12-12 16:59:29.000000000 -0500
--- Octant.h    2008-10-23 10:17:31.000000000 -0400
*************** public:
*** 60,66 ****
         */
        static int octant(const geom::Coordinate& p0, const geom::Coordinate& p1);
        static int octant(const geom::Coordinate* p0, const geom::Coordinate* p1) {
!               return octant(*p1, *p1);
        }
  };

--- 60,66 ----
         */
        static int octant(const geom::Coordinate& p0, const geom::Coordinate& p1);
        static int octant(const geom::Coordinate* p0, const geom::Coordinate* p1) {
!               return octant(*p0, *p1);
        }
  };

Reasoning:

• the other two octant methods use both parameters.
  • static int octant(double dx, double dy);
  • static int octant(const geom::Coordinate& p0, const geom::Coordinate& p1);
• the method in question calls octant(const geom::Coordinate& p0, const geom::Coordinate& p1), which subtracts p0 from p1 coordinates, and if the result is zero (as it would be when p0 == p1), an exception is thrown.

[warmerdam]

Denise,

Do you think you could prepare an extension for the unit tests that would actually identify this error, and confirm the fix? It could be something that calls this functionality fairly directly or something that invokes it indirectly via higher level services.

[denisem]

Sure, I'll give that a try.

[denisem]

Frank -

I tried adding it as it's own set of tests, but had problems getting the .deps/OctantTest.Po file generated; even after changing the makefiles it wasn't being picked up. So I added it as another test to tests/unit/noding/SegmentStringTest.cpp (since SegmentString class was calling Octant class already).

I will attach the changed SegmentStringTest.cpp file, and a diff file. Basically the new test is to call the two interfaces with the same parameters, and ensure that they have the same result.

Here's the run with the bug:

[root@localhost unit]# ./geos_unit geos::noding::SegmentString
===============================
  GEOS Test Suite Application
===============================

geos::noding::SegmentString: ....[5=F] 4


---> group: geos::noding::SegmentString, test: test<5>
     problem: assertion failed

Tests summary:
 - failures:1
 - passed: 4
[root@localhost unit]# 

And the run after the fix was applied:

[root@localhost unit]# ./geos_unit geos::noding::SegmentString
===============================
  GEOS Test Suite Application
===============================

geos::noding::SegmentString: ..... 5


Tests summary:
 - passed: 5
[root@localhost unit]# 

[pramsey]

Applied and verified in trunk, r2229 and in branch 3.0 r2230.