Skip to content

Commit

Permalink
Remove ConstructorOption from WKB parsing and GEOS
Browse files Browse the repository at this point in the history
  • Loading branch information
peterstace committed Sep 5, 2023
1 parent 6d22e3b commit 90155e5
Show file tree
Hide file tree
Showing 10 changed files with 58 additions and 56 deletions.
2 changes: 1 addition & 1 deletion geom/perf_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func BenchmarkUnmarshalWKB(b *testing.B) {
wkb := regularPolygon(XY{}, 1.0, sz).AsBinary()
b.ResetTimer()
for i := 0; i < b.N; i++ {
_, err := UnmarshalWKB(wkb, DisableAllValidations)
_, err := UnmarshalWKBWithoutValidation(wkb)
if err != nil {
b.Fatal(err)
}
Expand Down
2 changes: 1 addition & 1 deletion geom/wkb_bench_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ func BenchmarkWKBParse(b *testing.B) {
for i, tt := range validWKBCases {
b.Run(strconv.Itoa(i), func(b *testing.B) {
for n := 0; n < b.N; n++ {
_, err := geom.UnmarshalWKB(hexStringToBytes(b, tt.wkb), geom.DisableAllValidations)
_, err := geom.UnmarshalWKBWithoutValidation(hexStringToBytes(b, tt.wkb))
if err != nil {
panic(err)
}
Expand Down
37 changes: 25 additions & 12 deletions geom/wkb_parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,34 @@ import (
)

// UnmarshalWKB reads the Well Known Binary (WKB), and returns the
// corresponding Geometry.
func UnmarshalWKB(wkb []byte, opts ...ConstructorOption) (Geometry, error) {
// corresponding Geometry. The result has its geometry constraints validated.
func UnmarshalWKB(wkb []byte) (Geometry, error) {
g, err := UnmarshalWKBWithoutValidation(wkb)
if err != nil {
return Geometry{}, err
}
if err := g.Validate(); err != nil {
return Geometry{}, err
}
return g, nil
}

// UnmarshalWKB reads the Well Known Binary (WKB), and returns the
// corresponding Geometry. The result does not have its geometry constraints
// validated.
func UnmarshalWKBWithoutValidation(wkb []byte) (Geometry, error) {
// Note that we purposefully DON'T check for the presence of trailing
// bytes. There is nothing in the OGC spec indicating that trailing bytes
// are illegal. Some Esri software will add (useless) trailing bytes to
// their WKBs.
p := wkbParser{body: wkb, opts: opts}
p := wkbParser{body: wkb}
return p.run()
}

type wkbParser struct {
body []byte
bo byte
no bool
opts []ConstructorOption
}

func (p *wkbParser) run() (Geometry, error) {
Expand All @@ -37,7 +50,7 @@ func (p *wkbParser) run() (Geometry, error) {
}

func (p *wkbParser) inner() (Geometry, error) {
inner := wkbParser{body: p.body, opts: p.opts}
inner := wkbParser{body: p.body}
g, err := inner.run()
if err != nil {
return Geometry{}, err
Expand Down Expand Up @@ -207,7 +220,7 @@ func (p *wkbParser) parsePoint(ctype CoordinatesType) (Point, error) {
if math.IsNaN(c.X) || math.IsNaN(c.Y) {
return Point{}, wkbSyntaxError{"point contains mixed NaN values"}
}
return NewPoint(c, p.opts...)
return NewPointWithoutValidation(c), nil
}

func (p *wkbParser) parseLineString(ctype CoordinatesType) (LineString, error) {
Expand All @@ -233,7 +246,7 @@ func (p *wkbParser) parseLineString(ctype CoordinatesType) (LineString, error) {
copy(floats, bytesAsFloats(seqData))

seq := NewSequence(floats, ctype)
return NewLineString(seq, p.opts...)
return NewLineStringWithoutValidation(seq), nil
}

// bytesAsFloats reinterprets the bytes slice as a float64 slice in a similar
Expand Down Expand Up @@ -271,7 +284,7 @@ func (p *wkbParser) parsePolygon(ctype CoordinatesType) (Polygon, error) {
return Polygon{}, err
}
}
return NewPolygon(rings, p.opts...)
return NewPolygonWithoutValidation(rings), nil
}

func (p *wkbParser) parseMultiPoint(ctype CoordinatesType) (MultiPoint, error) {
Expand All @@ -293,7 +306,7 @@ func (p *wkbParser) parseMultiPoint(ctype CoordinatesType) (MultiPoint, error) {
}
pts[i] = geom.MustAsPoint()
}
return NewMultiPoint(pts, p.opts...), nil
return NewMultiPoint(pts), nil
}

func (p *wkbParser) parseMultiLineString(ctype CoordinatesType) (MultiLineString, error) {
Expand All @@ -315,7 +328,7 @@ func (p *wkbParser) parseMultiLineString(ctype CoordinatesType) (MultiLineString
}
lss[i] = geom.MustAsLineString()
}
return NewMultiLineString(lss, p.opts...), nil
return NewMultiLineString(lss), nil
}

func (p *wkbParser) parseMultiPolygon(ctype CoordinatesType) (MultiPolygon, error) {
Expand All @@ -337,7 +350,7 @@ func (p *wkbParser) parseMultiPolygon(ctype CoordinatesType) (MultiPolygon, erro
}
polys[i] = geom.MustAsPolygon()
}
return NewMultiPolygon(polys, p.opts...)
return NewMultiPolygon(polys)
}

func (p *wkbParser) parseGeometryCollection(ctype CoordinatesType) (GeometryCollection, error) {
Expand All @@ -355,5 +368,5 @@ func (p *wkbParser) parseGeometryCollection(ctype CoordinatesType) (GeometryColl
return GeometryCollection{}, err
}
}
return NewGeometryCollection(geoms, p.opts...), nil
return NewGeometryCollection(geoms), nil
}
10 changes: 5 additions & 5 deletions geos/benchmark_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,26 +22,26 @@ func regularPolygon(center geom.XY, radius float64, sides int) geom.Polygon {
}
coords[2*sides+0] = coords[0]
coords[2*sides+1] = coords[1]
ring, err := geom.NewLineString(geom.NewSequence(coords, geom.DimXY), geom.DisableAllValidations)
ring, err := geom.NewLineString(geom.NewSequence(coords, geom.DimXY))
if err != nil {
panic(err)
}
poly, err := geom.NewPolygon([]geom.LineString{ring}, geom.DisableAllValidations)
poly, err := geom.NewPolygon([]geom.LineString{ring})
if err != nil {
panic(err)
}
return poly
}

func BenchmarkIntersectionWithoutValidation(b *testing.B) {
func BenchmarkIntersection(b *testing.B) {
for _, sz := range []int{10, 100, 1000, 10000} {
b.Run(fmt.Sprintf("n=%d", sz), func(b *testing.B) {
inputA := regularPolygon(geom.XY{X: 0, Y: 0}, 1.0, sz).AsGeometry()
inputB := regularPolygon(geom.XY{X: 1, Y: 0}, 1.0, sz).AsGeometry()
b.ResetTimer()

for i := 0; i < b.N; i++ {
_, err := Intersection(inputA, inputB, geom.DisableAllValidations)
_, err := Intersection(inputA, inputB)
if err != nil {
b.Fatal(err)
}
Expand All @@ -57,7 +57,7 @@ func BenchmarkNoOp(b *testing.B) {
b.ResetTimer()

for i := 0; i < b.N; i++ {
_, err := noop(input, geom.DisableAllValidations)
_, err := noop(input)
if err != nil {
b.Fatal(err)
}
Expand Down
43 changes: 17 additions & 26 deletions geos/entrypoints.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,8 +179,8 @@ func Overlaps(a, b geom.Geometry) (bool, error) {
// Union returns a geometry that is the union of the input geometries.
// Formally, the returned geometry will contain a particular point X if and
// only if X is present in either geometry (or both).
func Union(a, b geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, error) {
g, err := binaryOpG(a, b, opts, func(ctx C.GEOSContextHandle_t, a, b *C.GEOSGeometry) *C.GEOSGeometry {
func Union(a, b geom.Geometry) (geom.Geometry, error) {
g, err := binaryOpG(a, b, func(ctx C.GEOSContextHandle_t, a, b *C.GEOSGeometry) *C.GEOSGeometry {
return C.GEOSUnion_r(ctx, a, b)
})
return g, wrap(err, "executing GEOSUnion_r")
Expand All @@ -189,8 +189,8 @@ func Union(a, b geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, e
// Intersection returns a geometry that is the intersection of the input
// geometries. Formally, the returned geometry will contain a particular point
// X if and only if X is present in both geometries.
func Intersection(a, b geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, error) {
g, err := binaryOpG(a, b, opts, func(ctx C.GEOSContextHandle_t, a, b *C.GEOSGeometry) *C.GEOSGeometry {
func Intersection(a, b geom.Geometry) (geom.Geometry, error) {
g, err := binaryOpG(a, b, func(ctx C.GEOSContextHandle_t, a, b *C.GEOSGeometry) *C.GEOSGeometry {
return C.GEOSIntersection_r(ctx, a, b)
})
return g, wrap(err, "executing GEOSIntersection_r")
Expand All @@ -204,7 +204,6 @@ type bufferOptionSet struct {
endCapStyle int
joinStyle int
mitreLimit float64
ctorOpts []geom.ConstructorOption
}

func newBufferOptionSet(opts []BufferOption) bufferOptionSet {
Expand Down Expand Up @@ -278,19 +277,11 @@ func BufferJoinStyleBevel() BufferOption {
}
}

// BufferConstructorOption sets constructor option that are used when
// reconstructing the buffered geometry that is returned from the GEOS lib.
func BufferConstructorOption(opts ...geom.ConstructorOption) BufferOption {
return func(bos *bufferOptionSet) {
bos.ctorOpts = append(bos.ctorOpts, opts...)
}
}

// Buffer returns a geometry that contains all points within the given radius
// of the input geometry.
func Buffer(g geom.Geometry, radius float64, opts ...BufferOption) (geom.Geometry, error) {
optSet := newBufferOptionSet(opts)
result, err := unaryOpG(g, optSet.ctorOpts, func(ctx C.GEOSContextHandle_t, gh *C.GEOSGeometry) *C.GEOSGeometry {
result, err := unaryOpG(g, func(ctx C.GEOSContextHandle_t, gh *C.GEOSGeometry) *C.GEOSGeometry {
return C.GEOSBufferWithStyle_r(
ctx, gh, C.double(radius),
C.int(optSet.quadSegments),
Expand All @@ -306,26 +297,26 @@ func Buffer(g geom.Geometry, radius float64, opts ...BufferOption) (geom.Geometr
// Douglas-Peucker algorithm. Topological invariants may not be maintained,
// e.g. polygons can collapse into linestrings, and holes in polygons may be
// lost.
func Simplify(g geom.Geometry, tolerance float64, opts ...geom.ConstructorOption) (geom.Geometry, error) {
result, err := unaryOpG(g, opts, func(ctx C.GEOSContextHandle_t, gh *C.GEOSGeometry) *C.GEOSGeometry {
func Simplify(g geom.Geometry, tolerance float64) (geom.Geometry, error) {
result, err := unaryOpG(g, func(ctx C.GEOSContextHandle_t, gh *C.GEOSGeometry) *C.GEOSGeometry {
return C.GEOSSimplify_r(ctx, gh, C.double(tolerance))
})
return result, wrap(err, "executing GEOSSimplify_r")
}

// Difference returns the geometry that represents the parts of input geometry
// A that are not part of input geometry B.
func Difference(a, b geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, error) {
result, err := binaryOpG(a, b, opts, func(ctx C.GEOSContextHandle_t, a, b *C.GEOSGeometry) *C.GEOSGeometry {
func Difference(a, b geom.Geometry) (geom.Geometry, error) {
result, err := binaryOpG(a, b, func(ctx C.GEOSContextHandle_t, a, b *C.GEOSGeometry) *C.GEOSGeometry {
return C.GEOSDifference_r(ctx, a, b)
})
return result, wrap(err, "executing GEOSDifference_r")
}

// SymmetricDifference returns the geometry that represents the parts of the
// input geometries that are not part of the other input geometry.
func SymmetricDifference(a, b geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, error) {
result, err := binaryOpG(a, b, opts, func(ctx C.GEOSContextHandle_t, a, b *C.GEOSGeometry) *C.GEOSGeometry {
func SymmetricDifference(a, b geom.Geometry) (geom.Geometry, error) {
result, err := binaryOpG(a, b, func(ctx C.GEOSContextHandle_t, a, b *C.GEOSGeometry) *C.GEOSGeometry {
return C.GEOSSymDifference_r(ctx, a, b)
})
return result, wrap(err, "executing GEOSSymDifference_r")
Expand All @@ -336,13 +327,13 @@ func SymmetricDifference(a, b geom.Geometry, opts ...geom.ConstructorOption) (ge
// geometry that is valid and similar (but not the same as) the original
// invalid geometry. If the input geometry is valid, then it is returned
// unaltered.
func MakeValid(g geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, error) {
func MakeValid(g geom.Geometry) (geom.Geometry, error) {
if C.MAKE_VALID_MISSING != 0 {
return geom.Geometry{}, unsupportedGEOSVersionError{
C.MAKE_VALID_MIN_VERSION, "MakeValid",
}
}
result, err := unaryOpG(g, opts, func(ctx C.GEOSContextHandle_t, g *C.GEOSGeometry) *C.GEOSGeometry {
result, err := unaryOpG(g, func(ctx C.GEOSContextHandle_t, g *C.GEOSGeometry) *C.GEOSGeometry {
return C.GEOSMakeValid_r(ctx, g)
})
return result, wrap(err, "executing GEOSMakeValid_r")
Expand All @@ -362,13 +353,13 @@ func MakeValid(g geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry,
// and return an error, but this is not guaranteed, and an invalid result may
// be returned without an error. It is the responsibility of the caller to
// ensure that the constraints are met before using this function.
func CoverageUnion(g geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, error) {
func CoverageUnion(g geom.Geometry) (geom.Geometry, error) {
if C.COVERAGE_UNION_MISSING != 0 {
return geom.Geometry{}, unsupportedGEOSVersionError{
C.COVERAGE_UNION_MIN_VERSION, "CoverageUnion",
}
}
result, err := unaryOpG(g, opts, func(ctx C.GEOSContextHandle_t, g *C.GEOSGeometry) *C.GEOSGeometry {
result, err := unaryOpG(g, func(ctx C.GEOSContextHandle_t, g *C.GEOSGeometry) *C.GEOSGeometry {
return C.GEOSCoverageUnion_r(ctx, g)
})
return result, wrap(err, "executing GEOSCoverageUnion_r")
Expand All @@ -377,8 +368,8 @@ func CoverageUnion(g geom.Geometry, opts ...geom.ConstructorOption) (geom.Geomet
// UnaryUnion is a single argument version of Union. It is most useful when
// supplied with a GeometryCollection, resulting in the union of the
// GeometryCollection's child geometries.
func UnaryUnion(g geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, error) {
result, err := unaryOpG(g, opts, func(ctx C.GEOSContextHandle_t, g *C.GEOSGeometry) *C.GEOSGeometry {
func UnaryUnion(g geom.Geometry) (geom.Geometry, error) {
result, err := unaryOpG(g, func(ctx C.GEOSContextHandle_t, g *C.GEOSGeometry) *C.GEOSGeometry {
return C.GEOSUnaryUnion_r(ctx, g)
})
return result, wrap(err, "executing GEOSUnaryUnion_r")
Expand Down
2 changes: 1 addition & 1 deletion geos/entrypoints_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,7 @@ type BinaryOperationTestCase struct {
Out string
}

func RunBinaryOperationTest(t *testing.T, fn func(a, b geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, error), cases []BinaryOperationTestCase) {
func RunBinaryOperationTest(t *testing.T, fn func(a, b geom.Geometry) (geom.Geometry, error), cases []BinaryOperationTestCase) {
for i, c := range cases {
t.Run(strconv.Itoa(i), func(t *testing.T) {
run := func(rev bool) func(t *testing.T) {
Expand Down
6 changes: 2 additions & 4 deletions geos/generic_operations.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ func binaryOpE(

func binaryOpG(
g1, g2 geom.Geometry,
opts []geom.ConstructorOption,
op func(C.GEOSContextHandle_t, *C.GEOSGeometry, *C.GEOSGeometry) *C.GEOSGeometry,
) (geom.Geometry, error) {
var result geom.Geometry
Expand All @@ -54,7 +53,7 @@ func binaryOpG(
}
defer C.GEOSGeom_destroy(resultGH)
var err error
result, err = h.decode(resultGH, opts)
result, err = h.decode(resultGH)
return wrap(err, "decoding result")
})
return result, err
Expand Down Expand Up @@ -95,7 +94,6 @@ func unaryOpE(g geom.Geometry, op func(*handle, *C.GEOSGeometry) error) error {

func unaryOpG(
g geom.Geometry,
opts []geom.ConstructorOption,
op func(C.GEOSContextHandle_t, *C.GEOSGeometry) *C.GEOSGeometry,
) (geom.Geometry, error) {
var result geom.Geometry
Expand All @@ -111,7 +109,7 @@ func unaryOpG(
defer C.GEOSGeom_destroy(resultGH)
}
var err error
result, err = h.decode(resultGH, opts)
result, err = h.decode(resultGH)
return wrap(err, "decoding result")
})
return result, err
Expand Down
8 changes: 4 additions & 4 deletions geos/handle.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ import (
// noop returns the geometry unaltered, via conversion to and from GEOS. This
// function is only for benchmarking purposes, hence it is not exported or used
// outside of benchmark tests.
func noop(g geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, error) {
result, err := unaryOpG(g, opts, func(ctx C.GEOSContextHandle_t, g *C.GEOSGeometry) *C.GEOSGeometry {
func noop(g geom.Geometry) (geom.Geometry, error) {
result, err := unaryOpG(g, func(ctx C.GEOSContextHandle_t, g *C.GEOSGeometry) *C.GEOSGeometry {
return C.noop(ctx, g)
})
return result, wrap(err, "executing noop")
Expand Down Expand Up @@ -158,7 +158,7 @@ func (h *handle) boolErr(c C.char) (bool, error) {
}
}

func (h *handle) decode(gh *C.GEOSGeometry, opts []geom.ConstructorOption) (geom.Geometry, error) {
func (h *handle) decode(gh *C.GEOSGeometry) (geom.Geometry, error) {
var (
isWKT C.char
size C.size_t
Expand All @@ -175,6 +175,6 @@ func (h *handle) decode(gh *C.GEOSGeometry, opts []geom.ConstructorOption) (geom
return g, wrap(err, "failed to unmarshal GEOS WKT result")
}
wkb := C.GoBytes(unsafe.Pointer(serialised), C.int(size))
g, err := geom.UnmarshalWKB(wkb, opts...)
g, err := geom.UnmarshalWKBWithoutValidation(wkb)
return g, wrap(err, "failed to unmarshal GEOS WKB result")
}
2 changes: 1 addition & 1 deletion internal/cmprefimpl/cmpgeos/extract_source.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func convertToGeometries(candidates []string) ([]geom.Geometry, error) {
if err != nil {
continue
}
g, err := geom.UnmarshalWKB(buf, geom.DisableAllValidations)
g, err := geom.UnmarshalWKBWithoutValidation(buf)
if err == nil {
geoms = append(geoms, g)
}
Expand Down
2 changes: 1 addition & 1 deletion internal/perf/set_op_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func BenchmarkSetOperation(b *testing.B) {
}

func adaptGEOSSetOp(
setOp func(_, _ geom.Geometry, _ ...geom.ConstructorOption) (geom.Geometry, error),
setOp func(_, _ geom.Geometry) (geom.Geometry, error),
) func(_, _ geom.Geometry) (geom.Geometry, error) {
return func(g1, g2 geom.Geometry) (geom.Geometry, error) {
return setOp(g1, g2)
Expand Down

0 comments on commit 90155e5

Please sign in to comment.