On Wed, Jul 12, 2006 at 12:59:59PM -0600, Charlie Savage wrote: > For what its worth, now that I've gone through the c api for the SWIG > bindings, here is some feedback: > > 1. The methods GEOSCoordSeq_getSize and GEOSCoordSeq_getDimensions > return their results by updating a pointer to a size_t parameter. Thus > like this: > > GEOSCoordSeq_getSize(const GEOSCoordSeq s, size_t *size); > > In contrast, these methods just return the result: > > GEOSGeom_getDimensions returns value, not as a parameter > GEOSGeom_getNumcoordinates > GEOSGeom_getNumInteriorRings > GEOSGeom_getNumGeometries > > Seems like an unneeded inconsistency.
Agreed. Unfortunately we really do not want to change any signature. The rationale for returning to a parameter was to handle exceptions, since these are inspecting-only methods it seems like GEOSCoordSeq_getSize and GEOSCoordSeq_getDimension are the weird ones > 2. The coordinate sequence api is extremely awkward to use. Its > verbose and results in fairly unreadable code. Like this: ... > Could we please expose a simplified for coordinate sequences in the c > api? Such as an "add" method, and while we are at it, shouldn't there > also be a remove method? We're in C land, no overloads. We might add addPoint2d(double, double) and addPoint3d(double, double, double) Yes, we can add remove (return 0 on exception, 1 on success). > 3. A lot of methods, like GEOSDisjoint, return a char. The char is 0 > for false, 1 for true and 2 for an exception. I took that to mean '0', > '1', '2'. In fact its not, its like this: > > (char)1 > > Thus if you look at the char is actually '%' (or some such thing). You > of course need to get the int result back by a typecast > > result = (int)GEOSDisjoint > > Maybe this could be better documented? It can be probably be changed to int w/out harm to existing code... What do other people think ? > 4. Buffer operation now takes quadrant segments. I had no idea what > that should be...hunting through the code it turns out to default to 8. > Do people use the quadrant parameter much? Seems like an unneeded bit > of complexity. At a minimum, can the default value be exposed as a > constant? That way the SWIG bindings can set a default value for the > buffer operation so users can just do geom.buffer(10) and can specify > the second parameter only if needed. Take a circle and split it by a vertical and an horizontal line. You get 4 quadrants. How many segments you want to end up in each quadrant when approximating a circle ? I suggest you define the constant internally to SWIG, if possible. > 5. Two operations that used to be exposed via SWIG are no longer: > > within_distance > equal_exact We can add equal_exact (see previous mail). We do have GEOSDistance(). > 6. Not very important, but the shape factory is not exposed via the c > api. Not sure if anyone uses it - its used in example.cpp. example.cpp is for C++ api, don't mess with it ! :) > 7. The envelopes is not exposed. I find that one quite useful when > dealing with bounding boxes - easier than having to work with a polygon > that represents a bounding box. That would mean defining another abstract data type, and some ops to manage it. Not a minor tweak. I'd postpone this to when we'll have indexes exposed (if ever :P) > Anyway, hope these comments are helpful. Very much, thanks! Will you also commit the missing bits ? --strk; _______________________________________________ geos-devel mailing list geos-devel@geos.refractions.net http://geos.refractions.net/mailman/listinfo/geos-devel
