Update Constructors with new functionality and test memory safety #136
Conversation
New createPolygon function allows creation of polygons without user specified holes. Polygon constructor added to convert a LinearRing to Polygon. Polygon constructor that takes a pointer limited by type.
Create a test file for GEOSGeom constructors found in geos_types.jl. Tests pass for Polygon constructed from vectors, but an "illegal instruction" error is thrown when one is constructed from a pointer. The error messages mentions destroyGeom. There are more tests below for constructing from a linear ring but these depend on the pointer constructor and thus cause a seg fault.
Adding cloneGeom around pointers sent to Polygon constructor that takes in a pointer has stopped the tests from seg faulting. All desired Polygon constructors and createPolygon functionality added and tested.
Typo in comments, remove magic numbers, change variable name
Added cloneGeom to Point constructor to stop seg fault and tested each method to construct a point.
Update existing multipoint constructors to clone pointers and add constructors to take in a vector of points and a point pointer.
Update LineString and MultiLineString constructors so that they use cloneGeom when needed and only accept appropriate type pointers. Added tests for all constructor functionality.
Add cloneGeom to LinearRing constructor and add tests for all functionality. Also refactor names and add comments.
Add cloneGeom to constructors, add MultiPolygon constructor from list of polygons, and test all functionality.
There was a problem hiding this comment.
The logic looks overall sound to me, thank you for the PRs, and really wonderful accompanying test suites! I only have minor nits about the coding style, and left a few comments. Happy to approve once the open comments are resolved.
There might be some ideas around improving the performance of constructing the geometries by dispatching on the geomTypeId, but those can be explored as future enhancements without breaking the API and doesn't have to be part of this PR.
Are you thinking of making subtypes of GEOSGeom for each |
|
Realized that this also addresses issue #87 |
I think the idea will be along the lines of dispatching on "geometrytraits" (like in yeesian/ArchGDAL.jl#316). |
|
@yeesian I had actually made the optional changes that you suggested, I was just waiting for a comment on the cloning with MultiPolygon so that I could do it all in one push. Should I still push the changes? I can also add comments on the multi* cloning. Please advise! Would I have to open a new PR to do that? |
|
Ah sorry I misunderstood what you intended when you resolved the comments, and was slow to respond here. Thank you for following up in #137 regardless! |
This finishes addressing issue #129 and issue #133. All constructors have been updated and tested. New functionality has been added for a few of the constructors. For example, MultiPolygons can now be constructed using lists of Polygons.
The one thing I wanted someone else to take a look at is my use of cloneGeom when making Multi* objects from lists of other objects. For example, MultiPoint on lines 46-50. I know that when using createCollection, the ownership of the elements that make up the collection is transferred to the collection object. So the ownership of the point objects is transferred to the collection. However, the collection is local in scope, and is cloned when it is made into a MultiPoint object. Therefore, I think the points should retain ownership over themselves when we exit from the constructor. However, I wanted someone else to check my logic. If that isn't true, then I think we need to clone each of the elements that make up the collection rather than the collection itself.
Also I may have gone overboard on testing since it is relatively simple code once the memory issue was figured out. Let me know if I should remove any.
Thanks!