Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add spoly(spoint[]) constructor function #99

Merged
merged 3 commits into from
Nov 7, 2023
Merged

Add spoly(spoint[]) constructor function #99

merged 3 commits into from
Nov 7, 2023

Conversation

vitcpp
Copy link
Contributor

@vitcpp vitcpp commented Nov 1, 2023

No description provided.

@vitcpp
Copy link
Contributor Author

vitcpp commented Nov 1, 2023

After merge of #96, I will update the docs.

esabol
esabol suggested changes Nov 1, 2023
View reviewed changes
pgs_polygon.sql.in Outdated
IMMUTABLE STRICT PARALLEL SAFE;

COMMENT ON FUNCTION spoly(spoint[]) IS
'Create spoly from an array of points.';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment should be creates spoly from an array of points (no capitalization, no period, and "creates" not "create") in order to match the other comments on the other functions.

pgs_polygon.sql.in Outdated
'Create spoly from array of points.
Two consecutive numbers among those present
refer to the same occurrence and cover its
latitude and longitude, respectively.';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't match the comment on the same function in PR #96, so be prepared to resolve this conflict after PR #96 is merged. I don't believe multi-line comments work correctly, which is why I shortened it to one line. The second sentence is poorly phrased anyway, imho.

src/polygon.c Outdated
ArrayType *inarr = PG_GETARG_ARRAYTYPE_P(0);
SPoint *points;

np = ArrayGetNItems(ARR_NDIM(inarr), ARR_DIMS(inarr));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose combining this line with line 957 and just making it

      int np = ArrayGetNItems(ARR_NDIM(inarr), ARR_DIMS(inarr));

src/polygon.c
@@ -60,7 +60,7 @@ PG_FUNCTION_INFO_V1(spheretrans_poly_inverse);
PG_FUNCTION_INFO_V1(spherepoly_add_point);
PG_FUNCTION_INFO_V1(spherepoly_add_points_finalize);
PG_FUNCTION_INFO_V1(spherepoly_is_convex);

PG_FUNCTION_INFO_V1(spherepoly_from_point_array);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering why you didn't add this new function to polygon.h? I did in PR #96 and am wondering if I shouldn't have?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@esabol Function declaration in a header is needed when you use this function in another translation unit (.c file). You just include this header with function declarations to be able to use these functions without any compiler warnings. The spherepoly_from_point_array function is an interface function that is searched (ldsym) and called by postgresql. It is not used in other .c sources. Once, some other functions are defined in .h file, I will add the declaration.

Furthermore, pgindent requires "extern" modificator for functions declarations. I have some plans to use pgindent to reformat .h files to satisfy the postgresql coding style.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vitcpp wrote:

Function declaration in a header is needed when you use this function in another translation unit (.c file).

Uh, yeah. I've been a professional C developer for over 28 years. I know what .h files are used for.

In my C projects, I put the prototype for every function in its corresponding .h file. The only times I don't are when I'm working on an API library and I need to be concerned about which functions are in the public API and which are meant only to be used internally. And even then I'd probably create a separate internal-only .h file for the prototypes of internal functions. Anyway, that's just my development philosophy, I guess. Apparently, you have a different philosophy, and that's fine.

Basically, I just want to know if I should remove the prototype for spherepoly_deg that I added to src/polygon.h in PR #96. It sounds like I should!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@esabol I'm sorry, I have no any intentions to teach anyone. I just wanted to clearly describe my position. My bad, a bucket with apples from me! :)

I agree with you to put the function declaration into .h file in pgsphere to follow the pgsphere code style. All other API functions are declared in .h files. I would also propose to add extern, but not in this PR. Pgindent will properly apply formatting to function declarations if it sees the extern keyword.

I will rebase and update my PR in according with your comments after the merge of #96.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, cool.

upgrade_scripts/pg_sphere--1.3.1--1.3.2.sql.in Outdated
IMMUTABLE STRICT PARALLEL SAFE;

COMMENT ON FUNCTION spoly(spoint[]) IS
'Create spoly from an array of points.';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change comment to creates spoly from an array of points (no capitalization, no period, and "creates" not "create") in order to match the other comments on the other functions.

@esabol esabol mentioned this pull request Nov 1, 2023
Closed
esabol
esabol reviewed Nov 1, 2023
View reviewed changes
sql/poly.sql Outdated

SELECT spoly(ARRAY[spoint_deg(0, 0), spoint_deg(10, 0)]);

SELECT spoly(ARRAY[spoint_deg(0, 0), spoint_deg(10, 0), spoint_deg(10,10)]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Change "10,10" to "10, 10".

esabol
esabol reviewed Nov 1, 2023
View reviewed changes
sql/poly.sql Outdated

SELECT spoly(ARRAY[spoint_deg(0, 0), spoint_deg(10, 0), spoint_deg(10,10)]);

SELECT spoly(ARRAY[spoint_deg(0, 0), spoint_deg(10, 0), spoint_deg(10,10), spoint_deg(0, 10)]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Change "10,10" to "10, 10".

esabol
esabol reviewed Nov 1, 2023
View reviewed changes
src/polygon.c Outdated

if (np < 3)
{
elog(ERROR, "spoly_deg: invalid number of arguments (must be >= 3)");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change "spoly_deg" to "spoly" (or "spherepoly_from_point_array"?).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would propose to use the SQL function name but I'm not sure how to get the name from function arguments. Furthermore, such change should be done for other functions as well.

I would propose to use the func (C99) or FUNCTION macro but not sure that MSVC supports it. Lets keep the old approach now.

esabol
esabol reviewed Nov 1, 2023
View reviewed changes
src/polygon.c Outdated

if (ARR_HASNULL(inarr))
{
elog(ERROR, "spoly_deg: input array is invalid because if has null values");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change "spoly_deg" to "spoly" (or "spherepoly_from_point_array"?).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please change "because if" to "because it" in this error message.

@vitcpp vitcpp force-pushed the spoly_constructor_with_point_array branch from 34132d3 to ce73075 Compare November 3, 2023 12:08
@vitcpp
Copy link
Contributor Author

vitcpp commented Nov 3, 2023 •
edited
Loading

Rebased, fixed problems after the review. The second commit will be squashed after completing the review.

@vitcpp vitcpp force-pushed the spoly_constructor_with_point_array branch from ce73075 to 595ecc9 Compare November 3, 2023 13:07
@vitcpp
Copy link
Contributor Author

vitcpp commented Nov 3, 2023

I updated the doc with the simple description of spoly and spoly_deg function. I used another approach to describe the functions to demonstrate some other approaches to describe functions. I didn't use funcsynopsis which is not suitable for describing SQL functions (funcsynopsis is for languages like C).

esabol
esabol approved these changes Nov 4, 2023
View reviewed changes
Copy link
Contributor

@esabol esabol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! Thank you so much for this contribution, @vitcpp !

@vitcpp vitcpp merged commit b4a6fe4 into postgrespro:master Nov 7, 2023
15 checks passed
@vitcpp vitcpp mentioned this pull request Nov 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@esabol esabol esabol approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vitcpp @esabol