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

Improve lwgeom_geos clarity #264

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

Conversation

Projects
None yet
5 participants
@dbaston
Member

dbaston commented Jul 5, 2018

This PR attempts to clarify some of the code in lwgeom_geos.c by removing function calls that require non-obvious arguments (which are often NULL or __func__) and replacing them with calls to variadic macros/functions.

dbaston added some commits Jul 4, 2018

Replace geos_clean and geos_clean_and_fail with variadic macros
This simplifies calling code by removing NULL and __func__ arguments
that must otherwise be passed to these functions.
Use GEOS2LWGEOM and GEOS_FAIL_AND_CLEAN to simplify return values
This commit is failing tests, apparently because of SRID issues. Need to
look at the individual cases and figure out if bugs need to be reported
to GEOS for SRID preservation issues.
Manually copy SRID to GEOS results
Possibly this can be unnecessary in the future; see
https://trac.osgeo.org/geos/ticket/896
Rename GEOS_CLEAN to GEOS_FREE
To avoid confusion with geometry cleaning

@dbaston dbaston requested a review from Komzpa Jul 5, 2018

* error in case of SRID mismatch. Intended to be called
* from RESULT_SRID macro */
static int32_t
get_result_srid(int count, const char* funcname, ...)

This comment has been minimized.

@Algunenano

Algunenano Jul 5, 2018

Member

Since sizeof returns size_t I'd rather have both count and i as size_t too.

@Algunenano

Algunenano Jul 5, 2018

Member

Since sizeof returns size_t I'd rather have both count and i as size_t too.

@pramsey

This comment has been minimized.

Show comment
Hide comment
@pramsey

pramsey Jul 5, 2018

Member

I like it, 👍

Member

pramsey commented Jul 5, 2018

I like it, 👍

@dbaston

This comment has been minimized.

Show comment
Hide comment
@dbaston

dbaston Jul 10, 2018

Member

@Komzpa do you want to review this or should I merge it in?

Member

dbaston commented Jul 10, 2018

@Komzpa do you want to review this or should I merge it in?

@@ -49,6 +51,44 @@ lwgeom_geos_error(const char* fmt, ...)
va_end(ap);
}
/* Destroy any non-null GEOSGeometry* pointers passed as arguments */
#define GEOS_FREE(...) \

This comment has been minimized.

@Komzpa

Komzpa Jul 10, 2018

Member

is "destroy" = "free"?

@Komzpa

Komzpa Jul 10, 2018

Member

is "destroy" = "free"?

@Komzpa

This comment has been minimized.

Show comment
Hide comment
@Komzpa

Komzpa Jul 10, 2018

Member

@dbaston I'm ok with merge of this. Only thing I'm at doubt of is scope of current freeze and thus if this should go to 3.0 or 2.5.

Member

Komzpa commented Jul 10, 2018

@dbaston I'm ok with merge of this. Only thing I'm at doubt of is scope of current freeze and thus if this should go to 3.0 or 2.5.

@dbaston

This comment has been minimized.

Show comment
Hide comment
@dbaston

dbaston Jul 10, 2018

Member

Thanks for looking, @Komzpa . I'm inclined to target 2.5 so that we don't have three very different versions of this file in the wild.

Member

dbaston commented Jul 10, 2018

Thanks for looking, @Komzpa . I'm inclined to target 2.5 so that we don't have three very different versions of this file in the wild.

@robe2

This comment has been minimized.

Show comment
Hide comment
@robe2

robe2 Jul 10, 2018

Member

Okay go ahead 👍

Member

robe2 commented Jul 10, 2018

Okay go ahead 👍

@strk strk closed this in 1141189 Jul 10, 2018

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