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

Use user provided CFLAGS in topology and address standardizer #280

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

Conversation

Projects
None yet
3 participants
@Algunenano
Member

Algunenano commented Jul 30, 2018

For both extensions, use the CFLAGS provided in the configure step.

For topology I've decided to not change the function type signatures to get this into 2.5, so I've left several TODOs as the implementation is buggy if more rows are turned that INT_MAX. I've tried to make it so if this happens only information would be lost instead of having undefined behaviour.

@Komzpa Komzpa requested a review from strk Jul 30, 2018

@Algunenano Algunenano changed the title from Use user provided CFLAGS in topology and address sanitizer to Use user provided CFLAGS in topology and address standardizer Jul 30, 2018

@dbaston

This comment has been minimized.

Show comment
Hide comment
@dbaston

dbaston Jul 30, 2018

Member

I don't see any issue changing the signatures for 2.5 (these aren't user-facing functions); others may disagree.

Member

dbaston commented Jul 30, 2018

I don't see any issue changing the signatures for 2.5 (these aren't user-facing functions); others may disagree.

@strk

I see many changes unrelated to the PR subject

@@ -188,6 +188,7 @@ typedef struct LWT_BE_CALLBACKS_T {
* @param ids an array of element identifiers
* @param numelems input/output parameter, pass number of node identifiers
* in the input array, gets number of node in output array.
* TODO: Should be uint64 to match SPI_processed

This comment has been minimized.

@strk

strk Jul 30, 2018

Member

We don't need a match, btw, as this module is PostgreSQL agnostic. Possibly we do need a bigger integer, to allow for returning more than 4 million faces

@strk

strk Jul 30, 2018

Member

We don't need a match, btw, as this module is PostgreSQL agnostic. Possibly we do need a bigger integer, to allow for returning more than 4 million faces

This comment has been minimized.

@Algunenano

Algunenano Jul 30, 2018

Member

True, it's design as platform agnostic, but I've only seen this used in the topology/ folder which is tightly tied to Postgres.

@Algunenano

Algunenano Jul 30, 2018

Member

True, it's design as platform agnostic, but I've only seen this used in the topology/ folder which is tightly tied to Postgres.

@Algunenano

This comment has been minimized.

Show comment
Hide comment
@Algunenano

Algunenano Jul 30, 2018

Member

I don't see any issue changing the signatures for 2.5 (these aren't user-facing functions); others may disagree.

I don't mind going either way.

I see many changes unrelated to the PR subject

The changes are to fix the warnings/errors that arise once the flags are correctly passed, since otherwise the builds will break.

Member

Algunenano commented Jul 30, 2018

I don't see any issue changing the signatures for 2.5 (these aren't user-facing functions); others may disagree.

I don't mind going either way.

I see many changes unrelated to the PR subject

The changes are to fix the warnings/errors that arise once the flags are correctly passed, since otherwise the builds will break.

@strk strk dismissed their stale review Jul 30, 2018

I'm ok with the changes

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Jul 30, 2018

Member

You'll find librttopo being a 1:1 mapping of the liblwgeom version of topology support, except that one is using the re-entrant GEOS API (we should do as well).

Member

strk commented Jul 30, 2018

You'll find librttopo being a 1:1 mapping of the liblwgeom version of topology support, except that one is using the re-entrant GEOS API (we should do as well).

@Algunenano

This comment has been minimized.

Show comment
Hide comment
@Algunenano

Algunenano Jul 30, 2018

Member

You'll find librttopo being a 1:1 mapping of the liblwgeom version of topology support, except that one is using the re-entrant GEOS API (we should do as well).

Nice to know.

Mainly to avoid introducing possible issues in a beta, I'm going to merge this as is and defer changing the types to 3.0. I'll open a ticket to serve as a reminder.

Member

Algunenano commented Jul 30, 2018

You'll find librttopo being a 1:1 mapping of the liblwgeom version of topology support, except that one is using the re-entrant GEOS API (we should do as well).

Nice to know.

Mainly to avoid introducing possible issues in a beta, I'm going to merge this as is and defer changing the types to 3.0. I'll open a ticket to serve as a reminder.

@strk strk closed this in a4581ec Jul 30, 2018

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