Make polygon comparison compare shell and holes #397
Conversation
tests/unit/geom/PolygonTest.cpp
Outdated
| auto geo = reader_.read("POLYGON ((0 0, 10 0, 10 10, 0 10, 0 0), (1 1, 1 2, 2 2, 2 1, 1 1))"); | ||
| ensure(geo != nullptr); | ||
|
|
||
| PolygonPtr poly = dynamic_cast<PolygonPtr>(geo.get()); |
There was a problem hiding this comment.
You're testing the Polygon implementation of Geometry::compareToSameClass, so if you want to reduce boilerplate, you can avoid lines like 630-631 (which really test the WKTReader). I would also take out lines like 622 (again, the goal is not to test the WKTReader) but it really comes down to preference.
There was a problem hiding this comment.
Sorry to be dense, but I'm not seeing how to avoid these two lines here?
auto geo = reader_.read("POLYGON ((0 0, 10 0, 10 10, 0 10, 0 0), (1 1, 1 2, 2 2, 2 1, 1 1))");
PolygonPtr poly = dynamic_cast<PolygonPtr>(geo.get());
(total noob to C++ managed pointers)
There was a problem hiding this comment.
Well, you can't avoid the first :)
But you can call geo->compareToSameClass(geo2.get()) directly; you don't need to explicitly cast it to a Polygon.
There was a problem hiding this comment.
Looks like I need to cast the left side, otherwise compareToSameClass is inaccessible. But no need to cast the function parameter, so that simplifies a little.
There was a problem hiding this comment.
That's interesting -- compareToSameClass is protected in Geometry but public in Polygon. I can't think of a good reason for that off hand -- I wonder if it's an oversight.
There was a problem hiding this comment.
Yeah, Point::compareToSameClass is also protected, so the Polygon variant being public is probably a 15-year old oversight. The alternative is to just call geo->compareTo(geo2.get()) which is really the public interface. Not asking you to change anything, just trying to explain.
There was a problem hiding this comment.
Thanks @dbaston for helping me understand! The comment on it implies that it was changed intentionally for Polygon class?
I think since we're specifically trying to test compareToSameClass here, the cast is OK to leave as is.
There was a problem hiding this comment.
That's interesting --
compareToSameClassisprotectedinGeometrybutpublicinPolygon. I can't think of a good reason for that off hand -- I wonder if it's an oversight.
Sure seems like an oversight/misunderstanding. Can this be fixed?
There was a problem hiding this comment.
Sure seems like an oversight/misunderstanding. Can this be fixed?
Do you mean make it protected and then test compareTo instead of compareToSameClass ?
There was a problem hiding this comment.
Do you mean make it
protectedand then testcompareToinstead ofcompareToSameClass?
Yes.
|
@dbaston thanks for the fast review and guidance here! |
|
Squashed and committed as 587049c. Thanks! |
This PR makes sure that `Polygon::compareToSameClass`` compares holes as well as outer shell and adds a
corresponding test.
Previously, the comparison was only based on comparing the outer shell.
Resolves #1099
This was adapted from JTS. Note that the first variant of JTS
compareToSameClasshas the same issue as was present here.I adapted another test in that unit test file to create the tests here; it was not obvious how to reduce the boilerplate in this case (i.e., simpler ways to instantiate from WKT here).