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.

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

contributing: Add .clang-format #2272

Merged
merged 1 commit into from
Dec 21, 2022
Merged

Conversation

nilason
Copy link
Contributor

@nilason nilason commented Mar 20, 2022

This is a suggestion to replace GNU indent with clang-format.

Attempting to bulk process the code base with util/grass_indent_ALL.sh as reported in #1630 (and noticed in #2270) a number of problem occurs. This is caused by limited functionality and lack of active development on GNU indent.
However, bulk processing with clang-format causes no error, thanks to the fact that it is backed by a full C and (!) C++ code parser.
Replacing the GRASS formatting tool to clang-format may be the better bet for the future.

Initially I put this up as a draft, for testing and discussion. I tried to get the settings as close as the present style guide, the one notable deviation (I could discover so far) is with alignment of trailing comments.

The .clang-format file is created with clang-format version 13.

Note: only the addition of the .clang-format is the ultimate goal for this PR. The added commit with all the formatted source files are for illustration only.

To test this locally:

find -E . -regex '.*\.(cpp|hpp|c|h)' -exec clang-format -style=file -i {} \;

for testing alternative settings see https://clang.llvm.org/docs/ClangFormatStyleOptions.html

For reference, description of the settings in util/grass_indent.sh:

-npro	Do not read `.indent.pro' files.
-bad	Force blank lines after the declarations.
-bap	Force blank lines after procedure bodies.
-bbb	Force blank lines before block comments.
-br	Put braces on line with if, etc.
-bli0	Indent braces n spaces.
-bls	Put braces on the line after struct declaration lines.
-cli0	Case label indent of n spaces.
-ncs	Do not put a space after cast operators.
-fc1	Format comments in the first column.
-hnl	Prefer to break long lines at the position of newlines in the input.
-i4	Set indentation level to n spaces.
-nbbo	Do not prefer to break long lines before boolean operators.
-nbc	Do not force newlines after commas in declarations.
-nbfda	Don't put each argument in a function declaration on a separate line.
-nbfde	--dont-break-function-decl-args-end
-ncdb	Do not put comment delimiters on blank lines.
-ncdw	Do not cuddle } and the while of a do {} while;.
-nce	Do not cuddle } and else.
-nfca	Do not format any comments.
-npcs	Do not put space after the function in function calls.
-nprs	Do not put a space after every '(' and before every ')'.
-npsl	Put the type of a procedure on the same line as its name.
-nsc	Do not put the `*' character at the left of comments.
-nsob	Do not swallow optional blank lines.
-saf	Put a space after each for.
-sai	Put a space after each if.
-saw	Put a space after each while.
-sbi0	Indent braces of a struct, union or enum N spaces.
-ss	On one-line for and while statements, force a blank before the semicolon.
--no-tabs	Use spaces instead of tabs.

@nilason nilason added the enhancement New feature or request label Mar 20, 2022
@nilason nilason added this to the 8.4.0 milestone Mar 20, 2022
@wenzeslaus
Copy link
Member

+1 given the advantages of clang-format over GNU Indent (also considering we may want to indent modern C++). My experience is that clang-format is not stable in between versions, so to check with exact version in the CI, we need to use DoozyX/clang-format-lint-action. We will also need to update it time to time like we did with Black before it was stable (and still will have to update but with less changes in the source code).

BTW, the actual formatting (formatting only) changes should go to .git-blame-ignore-revs (#1391) - add add to git-blame-ignore-revs to the PR and after merge, create another PR to update the file.

@nilason
Copy link
Contributor Author

nilason commented Mar 21, 2022

Just updated the .clang-format file. The closest ClangFormat style to ours is the one of LLVM, the file now reflects that and only the deviating settings are not commented out.

I also limited the number of formatted example files, for better and quicker overview online.

+1 given the advantages of clang-format over GNU Indent (also considering we may want to indent modern C++). My experience is that clang-format is not stable in between versions, so to check with exact version in the CI, we need to use DoozyX/clang-format-lint-action. We will also need to update it time to time like we did with Black before it was stable (and still will have to update but with less changes in the source code).

Yes, I'm aware of this circumstances with ClangFormat versions, but I think the advantages compared to GNU indent are worth it. You're right, Black is a good parallel.

@nilason
Copy link
Contributor Author

nilason commented Mar 21, 2022

@nilason nilason force-pushed the add_clang_format branch 2 times, most recently from 41b797a to 7800e48 Compare March 22, 2022 21:18
@nilason
Copy link
Contributor Author

nilason commented Mar 22, 2022

Fixed some more settings to get even closer to present "GRASS" style and updated the demo files accordingly.

GRASS style's BreakBeforeBraces is now custom, but to use one of the builtin brace breaking styles the following cases (and only those) need to be changed to the following:

Mozilla:

class UntypedStream
{

...

 } else {

Stroustrup:

enum rule_type {

...

struct Cell_head {

...

typedef union _dglHeapData {

...

extern "C" {

Linux:

enum AMI_stream_type {

...

struct Cell_head {

...

union {

...

class AMI_STREAM : public UntypedStream
{

...

 } else {

...

 extern "C" {
 

WebKit:

enum AMI_stream_type {

...

struct Cell_head {

...

union {

...

 } else {

...

extern "C" {

Even though I'm perfectly ok with present GRASS style, I must say I personally prefer to convert to Stroustrup brace breaking style.

@neteler
Copy link
Member

neteler commented Aug 28, 2022

@nilason would you mind to redo the reformatting afresh on top of the current state of "main"?

@nilason
Copy link
Contributor Author

nilason commented Aug 28, 2022

@nilason would you mind to redo the reformatting afresh on top of the current state of "main"?

Certainly! Hopefully this crazy busy summer will soon reach the end. I look forward to return to a more active contribution. :-)

@nilason
Copy link
Contributor Author

nilason commented Sep 18, 2022

@nilason would you mind to redo the reformatting afresh on top of the current state of "main"?

Just updated the formatted examples on current main.

@nilason nilason marked this pull request as ready for review December 16, 2022 09:24
@nilason nilason merged commit ff63ce3 into OSGeo:main Dec 21, 2022
@nilason nilason deleted the add_clang_format branch December 21, 2022 20:26
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
@wenzeslaus wenzeslaus changed the title Add .clang-format contributing: Add .clang-format Jun 6, 2023
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants