New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lwgeom_geos.c: clean up copy-paste #217

Closed
wants to merge 20 commits into
base: svn-trunk
from

Conversation

Projects
None yet
6 participants
@Komzpa
Member

Komzpa commented Feb 21, 2018

A large part of lwgeom_geos.c is error-handling copy-pasted from function to function.

There are a number of similar headers/footers and error clean up branches. "Let's do this on exit" or "let's validate input more thoroughly for paranoia mode" aren't easy as they will lead to increasingly more copy-paste.

Copy-paste artifacts in comments and error messages are also cleaned up.

Komzpa added some commits Feb 21, 2018

@@ -101,23 +101,18 @@ ptarray_from_GEOSCoordSeq(const GEOSCoordSequence* cs, char want3d)
/* Return an LWGEOM from a Geometry */
LWGEOM*
GEOS2LWGEOM(const GEOSGeometry* geom, char want3d)
GEOS2LWGEOM(const GEOSGeometry* geom, uint32_t want3d)

This comment has been minimized.

@dbaston

dbaston Feb 21, 2018

Member

Why change the type of the boolean?

This comment has been minimized.

@Komzpa

Komzpa Feb 21, 2018

Member

it's int in functions and char here.

LWGEOM* result;
uint32_t srid = get_result_srid(geom, NULL, __func__);
uint32_t is3d = FLAGS_GET_Z(geom->flags);
GEOSGeometry* g = init_geos_geometry_and_return_null();

This comment has been minimized.

@dbaston

dbaston Feb 21, 2018

Member

This seems like a confusing way to go from two lines to one.

This comment has been minimized.

@Komzpa

Komzpa Feb 21, 2018

Member

It's true. It looked good on first two functions though :)
Changed it to call initGEOS explicitly.

GEOSGeometry * LWGEOM2GEOS(const LWGEOM *g, int autofix);
GEOSGeometry * GBOX2GEOS(const GBOX *g);
GEOSGeometry * LWGEOM_GEOS_buildArea(const GEOSGeometry* geom_in);
LWGEOM* GEOS2LWGEOM(const GEOSGeometry* geom, uint32_t want3d);

This comment has been minimized.

@gardster

gardster Feb 21, 2018

Why char (one byte) -> uint32_t (4 bytes)?

This comment has been minimized.

@Komzpa

Komzpa Feb 21, 2018

Member

changed to uint8_t as it's used as boolean elsewhere in code.

if (lwgeom_is_empty(geom1)) return lwgeom_clone_deep(geom1);
/* Output encoder and sanity checker for GEOS wrappers */
inline static void
geos_clean(GEOSGeometry** g1, GEOSGeometry** g2, GEOSGeometry** g3)

This comment has been minimized.

@sbinq

sbinq Feb 21, 2018

Is that additional indirection necessary here - can we use * g1, .. instead of ** g1, .. ?

@Komzpa

This comment has been minimized.

Member

Komzpa commented Feb 21, 2018

@Algunenano can you take a look please? Tests pass, but maybe your clang sanitizers build can find something else to fix.

@Algunenano

There are several cases were const can be removed (returning a const int is the same as returning an int). Also, as srid can be positive I think int32_t is better than uint32_t (it's been used several times more than what I initially pointed out).

I get a new issue with the sanitizers (apart from the ones pending in my PR, and a whole lot of memory access issues coming from geos itself):

Direct leak of 40 byte(s) in 1 object(s) allocated from:
    #0 0x55699a33e3c1 in __interceptor_malloc (/home/raul/dev/public/postgis/liblwgeom/cunit/.libs/lt-cu_tester+0x13b3c1)
    #1 0x7fb9c41da4cd in lwcollection_clone_deep /home/raul/dev/public/postgis/liblwgeom/lwcollection.c:152:22
    #2 0x7fb9c41c794f in lwgeom_clone_deep /home/raul/dev/public/postgis/liblwgeom/lwgeom.c:543:20
    #3 0x7fb9c426a634 in lwgeom_linemerge /home/raul/dev/public/postgis/liblwgeom/lwgeom_geos.c:623:29
    #4 0x55699a38ef16 in test_geos_linemerge /home/raul/dev/public/postgis/liblwgeom/cunit/cu_geos.c:88:10
    #5 0x7fb9c3d26087 in run_single_test.constprop.5 /tmp/yaourt-tmp-raul/aur-cunit/src/CUnit-2.1-3/CUnit/Sources/Framework/TestRun.c:991

Missing a return in lwgeom_linemerge?

@@ -495,493 +488,316 @@ lwgeom_geos_version()
return ver;
}
LWGEOM*
lwgeom_normalize(const LWGEOM* geom1)
inline static const uint32_t

This comment has been minimized.

@Algunenano

Algunenano Feb 21, 2018

Member

const here is superfluous

This comment has been minimized.

@Algunenano

Algunenano Feb 21, 2018

Member

Also SRIDs can be negative, so int32_t is preferable (geom->srid already is int32_t).

This comment has been minimized.

@Komzpa

Komzpa Feb 21, 2018

Member

thanks, fixed.

if (!result)
/* Input decoder and sanity checker for GEOS wrappers */
inline static const uint32_t

This comment has been minimized.

@Algunenano

Algunenano Feb 21, 2018

Member

Same as above, const here doesn't mean anything.

LWGEOM*
lwgeom_intersection(const LWGEOM* geom1, const LWGEOM* geom2)
/* Output encoder and sanity checker for GEOS wrappers */
inline static const uint32_t

This comment has been minimized.

@Algunenano

Algunenano Feb 21, 2018

Member

Same with the const

lwgeom_intersection(const LWGEOM* geom1, const LWGEOM* geom2)
/* Output encoder and sanity checker for GEOS wrappers */
inline static const uint32_t
output_geos_as_lwgeom(GEOSGeometry** g, LWGEOM** geom, const uint32_t srid, const uint8_t is3d, const char* funcname)

This comment has been minimized.

@Algunenano

Algunenano Feb 21, 2018

Member

SRID should be positive (int32_t)

@@ -19,38 +19,31 @@
**********************************************************************
*
* Copyright 2011 Sandro Santilli <strk@kbt.io>
* Copyright 2018 Darafei Praliaskouski <me@komzpa.net>

This comment has been minimized.

@dbaston

dbaston Feb 21, 2018

Member

Maybe inquire on dev list what proper protocol for copyright is? I'm not sure myself, but the file has certainly been edited since 2018.

This comment has been minimized.

@Komzpa

Komzpa Feb 21, 2018

Member

It's strange. A significant part of the changes is @strk changing the copyrights and not updating years, I expect it to be an oversight that will easily be fixed with next commit of them :)

Copyright process is there in https://trac.osgeo.org/postgis/wiki/DevWikiComitGuidelines

This comment has been minimized.

@dbaston

dbaston Feb 21, 2018

Member

Thanks, I hadn't seen that link. People have different conceptions of "significant changes," I guess.

This comment has been minimized.

@robe2

robe2 Feb 22, 2018

Member

@Komzpa I guess what is considered major is up to interpretation.

snippet from guidelines
When substantial contributions are added to a file (such as substantial patches) the author/contributor should be added to the list of copyright holders for the file.
@dbaston Hmm I didn't make you read that guide :(

This comment has been minimized.

@dbaston

dbaston Feb 22, 2018

Member

Sorry for badly-written comment. I've obviously seen the commit guidelines, but didn't recall the snippet Regina posted. I suggested a dev list discussion because there are clearly different conceptions of when copyright should be asserted. I thought it was odd that you were claiming copyright for whitespace changes, but maybe that's just me.

lwerror("Error in GEOSNormalize: %s", lwgeom_geos_errmsg);
return NULL; /* never get here */
lwerror("%s: SRID is more than maximum (%u > %u)", funcname, geom1->srid, SRID_USER_MAXIMUM);
return SRID_INVALID;

This comment has been minimized.

@Algunenano

Algunenano Feb 22, 2018

Member

%d or "%" PRId32 would be better

@strk strk closed this in 8f5e42e Feb 22, 2018

@Komzpa Komzpa deleted the Komzpa:geos_fortify branch Feb 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment