Skip to content

Source files are moved into src subdirectory. #14

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

Merged
merged 3 commits into from
Jun 23, 2023

Conversation

vitcpp
Copy link
Contributor

@vitcpp vitcpp commented Jun 20, 2023

Source files were moved into the src subdirectory in order to avoid file mess in the project root directory.

@vitcpp
Copy link
Contributor Author

vitcpp commented Jun 20, 2023

@gmantele, @msdemlei Could you please review the patch. Is it suitable for you?

@vitcpp
Copy link
Contributor Author

vitcpp commented Jun 20, 2023

It seems some checks are failed. I will recheck it.

@vitcpp vitcpp force-pushed the create-src-subdir branch from 6302199 to 1793e40 Compare June 20, 2023 13:37
@vitcpp
Copy link
Contributor Author

vitcpp commented Jun 20, 2023

I've replaced angled braces with quotes in #include statement. It fixes the compilation with clang. Now tests are passed for versions 10+. Now everything should be ok.

Makefile Outdated
OBJS = src/sscan.o src/sparse.o src/sbuffer.o src/vector3d.o src/point.o \
src/euler.o src/circle.o src/line.o src/ellipse.o src/polygon.o \
src/path.o src/box.o src/output.o src/gq_cache.o src/gist.o src/key.o \
src/gnomo.o src/healpix.o src/moc.o src/process_moc.o healpix_bare/healpix_bare.o \
Copy link
Contributor

Choose a reason for hiding this comment

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

I would wrap this line, but that's a minor nit.

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 thank you for the review. I've added one more commit with wrapping some lines. I've also aligned identation of some lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the indentation of lines 8 and 9 look different from the indentation of lines 10 and 11 now. Are you using a different number of tabs on these lines to indent?

Copy link
Contributor Author

@vitcpp vitcpp Jun 21, 2023

Choose a reason for hiding this comment

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

@esabol I use postgresql code formatting conventions where tab width is 4 and identation is applied using tabs (https://www.postgresql.org/docs/current/source-format.html). It seems spaces are used for identation of untouched lines but I used tabs for identation. It is why there was the difference. I'm going to replace some spaces with tabs. The result will be like this:

image

But I will look ugly if to set 8 spaces in tab. I propose to follow postgresql conventions and to use tabs with width 4 for identation. What do you think?

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 Finally, I've decided to wrap the long line and nothing more. All my previous explanations seems to be wrong because postgresql coding standards are related to source files. Sorry. I propose to reformat Makefile in a separate PR later because there are different formatting styles used inside - the mix of spaces and tabs.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think?

My comment was just that lines 8-11 should be consistent in how they are indented.

I think tabs are expected to be 8 spaces wide in Makefiles, but I'm not 100% sure.

Continuation lines like this should be indented with at least one tab.

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 think I've done what you proposed. Now indentation is consistent for these lines. I apologize I have made some noise here. We can go further If everything is ok now. Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks perfect to me, @vitcpp ! Thank you!

@vitcpp vitcpp force-pushed the create-src-subdir branch from 578f98e to f92829b Compare June 21, 2023 19:25
@vitcpp vitcpp force-pushed the create-src-subdir branch from f92829b to 9b1153c Compare June 21, 2023 19:41
@msdemlei
Copy link
Contributor

msdemlei commented Jun 22, 2023 via email

@vitcpp
Copy link
Contributor Author

vitcpp commented Jun 22, 2023

Dear All, thank you! I will merge it tomorrow if no objections appear.

@vitcpp vitcpp merged commit 001cee8 into postgrespro:master Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants