[geos-devel] c api feedback

[strk at refractions.net]

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;