[GRASS-dev] Proposal for using ClangFormat, replacing GNU indent, for C/C++ code formatting

Dear All,

I have put up a PR [1] with the intent to add a “.clang-format” file and ultimately replace the use of “GNU indent” [2] with ClangFormat [3] for formatting GRASS’ source code. ClangFormat, using the Clang compiler parser, is able to format any valid code in both C and C++. (In contrast to numerous problems and limitations with GNU indent).
I’ve had the impression that this suggestion in general is a welcome one and not particularly controversial.

I have initially made the '.clang-format' to mirror as close as possible the settings in 'utils/grass_indent.sh' [4].

However, I'd like to propose changes to the (ClangFormat’s) “BreakBeforeBraces” rules [5]. The "GRASS"-style is a modified version of the GNU style. I think it would be preferable to simplify these rules with the so-called “Stroustrup” style, which is as close as it gets to K&R (in this regard) and also gives a slightly more compact code vertically. It can be described in short: braces start on new line *only* after functions, and 'else' and 'catch' start on new line after previous closing brace.

For example:

int f(void)
{
    if (...) {
        ...
    }
    else {
        ...
    }
}

All other cases -- enums, structs etc. -- attaches the starting brace, e.g.:

struct s {
    ...
}

Please, check out the formatted example files in the PR [6] to see what the proposed changes would look like with the "Stroustrup" BreakBeforeBraces-rules (note: the PR aims to only add the '.clang-format' file).

I have intentionally left the setting of "ReflowComments" to the default 'true', which cleverly reformats comments extending the 80 columns. This causes some (aesthetic) issues with trailing Doxygen comments in mainly the in the 'include/grass/ and 'lib/' directories, which probably necessitates some initial manual work. On the other hand, a batch format of all module code will likely go pretty smoothly.

TO THIS INTENT, to finally solve the long extended problem with -- or lack of -- uniformly formatted code and not kicking this stone further down the road, I suggest the following:

1. We adapt the formatting policy using ClangFormat.

2. We implement "BreakBeforeBraces" rules according the "Stroustrup" style.

3. If there are no objections raised within a two weeks period, say until December 18, either to points 1 and/or 2 or even to this proposed deadline, the PR [1] will be merged and work can start on source code formatting.

4. Any changes decided upon ought to be added to https://trac.osgeo.org/grass/wiki/Submitting/C

Cheers,
Nicklas

[1] https://github.com/OSGeo/grass/pull/2272
[2] https://www.gnu.org/software/indent/
[3] https://clang.llvm.org/docs/ClangFormat.html
[4] https://github.com/OSGeo/grass/blob/main/utils/grass_indent.sh
[5] https://clang.llvm.org/docs/ClangFormatStyleOptions.html
[6] https://github.com/OSGeo/grass/pull/2272/files

On Mon, 5 Dec 2022 at 08:07, Nicklas Larsson via grass-dev <grass-dev@lists.osgeo.org> wrote:

  1. We adapt the formatting policy using ClangFormat.

+1

I would prefer if formatting changes from GNU indent which can be done separately are done separately. For Python & Black, I did that together but 1) in multiple PRs and 2) we lacked any formatting, so it was a little different. Things like ReflowComments seem like a good second round. (Sorry, I’m a little confused about your intention with unused .clang-format and what will be in there.)

  1. We implement “BreakBeforeBraces” rules according the “Stroustrup” style.

Given the lack of unified standard in C and C++, I’m for making choices which bring the formatting closer to Python and Black which Stroustrup seems to do.

Related to that, the in-line comments in the PR are modified to have one space after the code while Black, i.e., our Python code, has two.

  1. If there are no objections raised within a two weeks period, say until December 18, either to points 1 and/or 2 or even to this proposed deadline, the PR [1] will be merged and work can start on source code formatting.

My approach with Black was to do config+formatting+CI at once which has the disadvantage of adding the commit with config+CI into .git-blame-ignore-revs, but a comment in that file warns about that and it is exactly what will happen when we change the formatting later on with active CI (the formatting will have to go in together with config and CI changes).

  1. Any changes decided upon ought to be added to https://trac.osgeo.org/grass/wiki/Submitting/C

Yes. FYI, although details are unclear, the long-term plan for these documents is to move them to code (based on a discussion at a PSC meeting).

Best,
Vaclav

Cheers,
Nicklas

[1] https://github.com/OSGeo/grass/pull/2272
[2] https://www.gnu.org/software/indent/
[3] https://clang.llvm.org/docs/ClangFormat.html
[4] https://github.com/OSGeo/grass/blob/main/utils/grass_indent.sh
[5] https://clang.llvm.org/docs/ClangFormatStyleOptions.html
[6] https://github.com/OSGeo/grass/pull/2272/files


grass-dev mailing list
grass-dev@lists.osgeo.org
https://lists.osgeo.org/mailman/listinfo/grass-dev

On 7 Dec 2022, at 02:51, Vaclav Petras <wenzeslaus@gmail.com> wrote:

On Mon, 5 Dec 2022 at 08:07, Nicklas Larsson via grass-dev <grass-dev@lists.osgeo.org> wrote:

  1. We adapt the formatting policy using ClangFormat.

+1

I would prefer if formatting changes from GNU indent which can be done separately are done separately.

I don’t quite follow you.

For Python & Black, I did that together but 1) in multiple PRs and 2) we lacked any formatting, so it was a little different. Things like ReflowComments seem like a good second round. (Sorry, I’m a little confused about your intention with unused .clang-format and what will be in there.)

My intention is simple. First agree on style and add the .clang-format file to source and only after that start the actual formatting work.

I strongly advice to make visual supervision on formatting changes before merging. For that reason alone the whole source base should be formatted in batches of about 500 files each to be manageable. In total there are ca. 3360 files. The directories ‘include’ and ‘lib’ may need special attention because of the Doxygen comments, all the other will be very easily processed. In all this will need some 5-8 PRs.

Directory #files


lib 1199
include 103
db 120
display 109
general 67
imagery 379
misc 13
ps 84
raster 774
raster3d 100
temporal 1
utils 2
vector 408
visualization 2

3361

I’d say we should strive to limit the formatting changes of a single file to minimum, i.e. to 1 (as in one) time. Therefore any changes like ReflowComments should be done in one go. (Also, I’m happy to go through all those file once, but not twice.)

  1. We implement “BreakBeforeBraces” rules according the “Stroustrup” style.

Related to that, the in-line comments in the PR are modified to have one space after the code while Black, i.e., our Python code, has two.

Well, the two are distinct different languages with different history. Black also imposes 88 col line width, not 80. I’d say we keep the ClangFormat defaults as long as there is no strong argument against. In the case with SpacesBeforeTrailingComments set to 1: do not forget that C-style comments take at least 6 characters! E.g. /* comment */. And that is not even a Doxygen doc comment.

I understand there is agreement on using the .clang-format formatting rules suggested with [1], which I just merged.

I have formatted the whole source base with clang-format v.15.0.6, in 7 different PRs [2-8]. I will start merging them tomorrow if there are no objections.

I have also filed a PR [9] which adds a CI check for clang-format errors.

Installing clang-format is perhaps most easily done with:
python -m pip install 'clang-format==15.0.6'

Formatting may be done with something like (following works on Mac):
find -E . -regex '.*\.(cpp|hpp|c|h)' -exec clang-format -i {} \+

Contribution rules must be updated, I will start putting up a draft ASAP.
Valuable inspiration may be taken from e.g. GDAL [10] and libtiff [11].

Cheers,
   Nicklas

Add .clang-format:
[1] https://github.com/OSGeo/grass/pull/2272

Format files:
[2] https://github.com/OSGeo/grass/pull/2709
[3] https://github.com/OSGeo/grass/pull/2710
[4] https://github.com/OSGeo/grass/pull/2711
[5] https://github.com/OSGeo/grass/pull/2712
[6] https://github.com/OSGeo/grass/pull/2713
[7] https://github.com/OSGeo/grass/pull/2714
[8] https://github.com/OSGeo/grass/pull/2715

CI check:
[9] https://github.com/OSGeo/grass/pull/2716

[10] https://lists.osgeo.org/pipermail/gdal-dev/2022-December/056658.html
[11] https://gitlab.com/libtiff/libtiff/-/merge_requests/431/diffs?commit_id=01cc265a7c7f24de14cda6dc40c960eb8c3c68bb

On 5 Dec 2022, at 14:06, Nicklas Larsson via grass-dev <grass-dev@lists.osgeo.org> wrote:

Dear All,

I have put up a PR [1] with the intent to add a “.clang-format” file and ultimately replace the use of “GNU indent” [2] with ClangFormat [3] for formatting GRASS’ source code. ClangFormat, using the Clang compiler parser, is able to format any valid code in both C and C++. (In contrast to numerous problems and limitations with GNU indent).
I’ve had the impression that this suggestion in general is a welcome one and not particularly controversial.

I have initially made the '.clang-format' to mirror as close as possible the settings in 'utils/grass_indent.sh' [4].

However, I'd like to propose changes to the (ClangFormat’s) “BreakBeforeBraces” rules [5]. The "GRASS"-style is a modified version of the GNU style. I think it would be preferable to simplify these rules with the so-called “Stroustrup” style, which is as close as it gets to K&R (in this regard) and also gives a slightly more compact code vertically. It can be described in short: braces start on new line *only* after functions, and 'else' and 'catch' start on new line after previous closing brace.

For example:

int f(void)
{
   if (...) {
       ...
   }
   else {
       ...
   }
}

All other cases -- enums, structs etc. -- attaches the starting brace, e.g.:

struct s {
   ...
}

Please, check out the formatted example files in the PR [6] to see what the proposed changes would look like with the "Stroustrup" BreakBeforeBraces-rules (note: the PR aims to only add the '.clang-format' file).

I have intentionally left the setting of "ReflowComments" to the default 'true', which cleverly reformats comments extending the 80 columns. This causes some (aesthetic) issues with trailing Doxygen comments in mainly the in the 'include/grass/ and 'lib/' directories, which probably necessitates some initial manual work. On the other hand, a batch format of all module code will likely go pretty smoothly.

TO THIS INTENT, to finally solve the long extended problem with -- or lack of -- uniformly formatted code and not kicking this stone further down the road, I suggest the following:

1. We adapt the formatting policy using ClangFormat.

2. We implement "BreakBeforeBraces" rules according the "Stroustrup" style.

3. If there are no objections raised within a two weeks period, say until December 18, either to points 1 and/or 2 or even to this proposed deadline, the PR [1] will be merged and work can start on source code formatting.

4. Any changes decided upon ought to be added to https://trac.osgeo.org/grass/wiki/Submitting/C

Cheers,
Nicklas

[1] https://github.com/OSGeo/grass/pull/2272
[2] https://www.gnu.org/software/indent/
[3] https://clang.llvm.org/docs/ClangFormat.html
[4] https://github.com/OSGeo/grass/blob/main/utils/grass_indent.sh
[5] https://clang.llvm.org/docs/ClangFormatStyleOptions.html
[6] https://github.com/OSGeo/grass/pull/2272/files

_______________________________________________
grass-dev mailing list
grass-dev@lists.osgeo.org
https://lists.osgeo.org/mailman/listinfo/grass-dev

Hi Nicklas,

On Wed, Dec 21, 2022 at 9:25 PM Nicklas Larsson via grass-dev
<grass-dev@lists.osgeo.org> wrote:

I understand there is agreement on using the .clang-format formatting rules suggested with [1], which I just merged.

I have formatted the whole source base with clang-format v.15.0.6, in 7 different PRs [2-8]. I will start merging them tomorrow if there are no objections.

I have also filed a PR [9] which adds a CI check for clang-format errors.

Thanks for your efforts on the code reformatting!

Installing clang-format is perhaps most easily done with:
python -m pip install 'clang-format==15.0.6'

Formatting may be done with something like (following works on Mac):
find -E . -regex '.*\.(cpp|hpp|c|h)' -exec clang-format -i {} \+

... it fails on Linux, though

find: unknown predicate `-E')

Contribution rules must be updated, I will start putting up a draft ASAP.

I am trying to fix the conflicts in
https://github.com/OSGeo/grass/pull/2684

Conflicting files:

include/grass/iostream/mm.h
lib/db/dbmi_base/dbmscap.c
lib/external/ccmath/ccmath.h
lib/gis/spawn.c
lib/gis/user_config.c
lib/iostream/rtimer.cpp
lib/pngdriver/graph_set.c
lib/rst/interp_float/point2d.c
raster/r.terraflow/filldepr.cpp
raster/r.terraflow/flow.cpp
raster/r.terraflow/main.cpp
raster/r.viewshed/statusstructure.cpp

The reason will be the missing clang-format update which I don't know
how to apply on Linux.

Markus

PS: If easy on Mac, feel free to directly push an update to the PR :slight_smile:

Markus,

On 2 Jan 2023, at 13:48, Markus Neteler <neteler@osgeo.org> wrote:

Hi Nicklas,

On Wed, Dec 21, 2022 at 9:25 PM Nicklas Larsson via grass-dev
<grass-dev@lists.osgeo.org> wrote:

I understand there is agreement on using the .clang-format formatting rules suggested with [1], which I just merged.

I have formatted the whole source base with clang-format v.15.0.6, in 7 different PRs [2-8]. I will start merging them tomorrow if there are no objections.

I have also filed a PR [9] which adds a CI check for clang-format errors.

Thanks for your efforts on the code reformatting!

:slight_smile:

Installing clang-format is perhaps most easily done with:
python -m pip install ‘clang-format==15.0.6’

Formatting may be done with something like (following works on Mac):
find -E . -regex ‘.*.(cpp|hpp|c|h)’ -exec clang-format -i {} +

… it fails on Linux, though

find: unknown predicate `-E’)

I was pretty sure this would deviate from Mac/BSD on Linux systems:

Try something like (untested):

find . -regex ‘.*.(cpp|hpp|c|h)’ -exec clang-format -i {} ;

or manually with:
clang-format -I

Contribution rules must be updated, I will start putting up a draft ASAP.

I am trying to fix the conflicts in
https://github.com/OSGeo/grass/pull/2684

Conflicting files:

include/grass/iostream/mm.h
lib/db/dbmi_base/dbmscap.c
lib/external/ccmath/ccmath.h
lib/gis/spawn.c
lib/gis/user_config.c
lib/iostream/rtimer.cpp
lib/pngdriver/graph_set.c
lib/rst/interp_float/point2d.c
raster/r.terraflow/filldepr.cpp
raster/r.terraflow/flow.cpp
raster/r.terraflow/main.cpp
raster/r.viewshed/statusstructure.cpp

The reason will be the missing clang-format update which I don’t know
how to apply on Linux.

Not quite sure what you mean. Did you try:

python -m pip install 'clang-format==15.0.6’

But I suspect any version 15 will do, perhaps even v. 14.

A tip to use the .clang-format file from main for branches without it:

  1. git checkout main
  2. cp .clang-forrmat …/.clang-format
  3. check out branch
  4. clang-format searches upwards in dir hierarchy for next ‘.clang-format’ file
  5. run clang-format from grass source dir

I’d suggest you use pre-commit so that clang-format is automatically run on git commit operations like we have done with GDAL. Then it is a no-brainer to do changes.

You need to add a .pre-commit-config.yaml at the root of the repository (only the part referencing clang-format at https://github.com/OSGeo/gdal/blob/master/.pre-commit-config.yaml#L30 is relevant for you):

https://github.com/OSGeo/gdal/blob/master/.pre-commit-config.yaml

Once that file is in place:

  • “pip install pre-commit” : just once

  • “pre-commit install”: just once per repository

Cf https://gdal.org/development/dev_practices.html#commit-hooks

Even

···

Le 02/01/2023 à 19:20, Nicklas Larsson via grass-dev a écrit :

Markus,

On 2 Jan 2023, at 13:48, Markus Neteler <neteler@osgeo.org> wrote:

Hi Nicklas,

On Wed, Dec 21, 2022 at 9:25 PM Nicklas Larsson via grass-dev
<grass-dev@lists.osgeo.org> wrote:

I understand there is agreement on using the .clang-format formatting rules suggested with [1], which I just merged.

I have formatted the whole source base with clang-format v.15.0.6, in 7 different PRs [2-8]. I will start merging them tomorrow if there are no objections.

I have also filed a PR [9] which adds a CI check for clang-format errors.

Thanks for your efforts on the code reformatting!

:slight_smile:

Installing clang-format is perhaps most easily done with:
python -m pip install ‘clang-format==15.0.6’

Formatting may be done with something like (following works on Mac):
find -E . -regex ‘.*.(cpp|hpp|c|h)’ -exec clang-format -i {} +

… it fails on Linux, though

find: unknown predicate `-E’)

I was pretty sure this would deviate from Mac/BSD on Linux systems:

Try something like (untested):

find . -regex ‘.*.(cpp|hpp|c|h)’ -exec clang-format -i {} ;

or manually with:
clang-format -I

Contribution rules must be updated, I will start putting up a draft ASAP.

I am trying to fix the conflicts in
https://github.com/OSGeo/grass/pull/2684

Conflicting files:

include/grass/iostream/mm.h
lib/db/dbmi_base/dbmscap.c
lib/external/ccmath/ccmath.h
lib/gis/spawn.c
lib/gis/user_config.c
lib/iostream/rtimer.cpp
lib/pngdriver/graph_set.c
lib/rst/interp_float/point2d.c
raster/r.terraflow/filldepr.cpp
raster/r.terraflow/flow.cpp
raster/r.terraflow/main.cpp
raster/r.viewshed/statusstructure.cpp

The reason will be the missing clang-format update which I don’t know
how to apply on Linux.

Not quite sure what you mean. Did you try:

python -m pip install 'clang-format==15.0.6’

But I suspect any version 15 will do, perhaps even v. 14.

A tip to use the .clang-format file from main for branches without it:

  1. git checkout main
  2. cp .clang-forrmat …/.clang-format
  3. check out branch
  4. clang-format searches upwards in dir hierarchy for next ‘.clang-format’ file
  5. run clang-format from grass source dir
_______________________________________________
grass-dev mailing list
[grass-dev@lists.osgeo.org](mailto:grass-dev@lists.osgeo.org)
[https://lists.osgeo.org/mailman/listinfo/grass-dev](https://lists.osgeo.org/mailman/listinfo/grass-dev)

-- 
[http://www.spatialys.com](http://www.spatialys.com)
My software is free, but my time generally not.

On Mon, Jan 2, 2023 at 7:20 PM Nicklas Larsson <n_larsson@yahoo.com> wrote:

On 2 Jan 2023, at 13:48, Markus Neteler <neteler@osgeo.org> wrote:

On Wed, Dec 21, 2022 at 9:25 PM Nicklas Larsson via grass-dev

Installing clang-format is perhaps most easily done with:
python -m pip install 'clang-format==15.0.6'

Formatting may be done with something like (following works on Mac):
find -E . -regex '.*\.(cpp|hpp|c|h)' -exec clang-format -i {} \+

... it fails on Linux

Try something like (untested):

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

That seems to work!

or manually with:
clang-format -I <file>

[...]

I am trying to fix the conflicts in
https://github.com/OSGeo/grass/pull/2684

Conflicting files:

include/grass/iostream/mm.h
lib/db/dbmi_base/dbmscap.c
lib/external/ccmath/ccmath.h
lib/gis/spawn.c
lib/gis/user_config.c
lib/iostream/rtimer.cpp
lib/pngdriver/graph_set.c
lib/rst/interp_float/point2d.c
raster/r.terraflow/filldepr.cpp
raster/r.terraflow/flow.cpp
raster/r.terraflow/main.cpp
raster/r.viewshed/statusstructure.cpp

The reason will be the missing clang-format update which I don't know
how to apply on Linux.

Not quite sure what you mean.

Probably I was confused and it needs a rebase...

Did you try:

python -m pip install 'clang-format==15.0.6’

But I suspect any version 15 will do, perhaps even v. 14.

A tip to use the .clang-format file from main for branches without it:

... this I was looking for (sorry for my unclear wish)!

1. git checkout main
2. cp .clang-forrmat ../.clang-format
3. check out branch
4. clang-format searches upwards in dir hierarchy for next ‘.clang-format’ file
5. run clang-format from grass source dir

Will try.

Thanks,
Markus

Even,
Thanks for the pointer to pre-commit!

All,

The complete GRASS source is now formatted with ClangFormat using the settings of the added ‘.clang-format’ file.

I have also added a '.pre-commit-config.yaml’, which facilitates the convenient use of ‘pre-commit’ [1].

The pre-commit hooks now supported are:

  • clang-format (for C, C++, JavaScript, JSON, Objective-C)

  • trailing-whitespace

  • end-of-file-fixer

  • markdownlint

  • black

  • flake8

For all developers, I strongly recommend installing and using pre-commit. It makes life so much easier and unloads considerable amount of unnecessary work on the CI runners (oh, Black failed, now ClangFormat too, push again…).

The submitting guides on the wiki has been converted to markdown and are awaiting the final destination in source repo [2]. After merge they need to be updated accordingly to latest developments.

The question now is, are there any objections to do the same treatment to grass-addons)?
There are three PRs in-store [3-5] to make that happen.

Cheers,
Nicklas

[1] https://pre-commit.com
[2] https://github.com/OSGeo/grass/pull/2765
[3] https://github.com/OSGeo/grass-addons/pull/852
[4] https://github.com/OSGeo/grass-addons/pull/853
[5] https://github.com/OSGeo/grass-addons/pull/854

···

Le 02/01/2023 à 19:20, Nicklas Larsson via grass-dev a écrit :

Markus,

On 2 Jan 2023, at 13:48, Markus Neteler <neteler@osgeo.org> wrote:

Hi Nicklas,

On Wed, Dec 21, 2022 at 9:25 PM Nicklas Larsson via grass-dev
<grass-dev@lists.osgeo.org> wrote:

I understand there is agreement on using the .clang-format formatting rules suggested with [1], which I just merged.

I have formatted the whole source base with clang-format v.15.0.6, in 7 different PRs [2-8]. I will start merging them tomorrow if there are no objections.

I have also filed a PR [9] which adds a CI check for clang-format errors.

Thanks for your efforts on the code reformatting!

:slight_smile:

Installing clang-format is perhaps most easily done with:
python -m pip install ‘clang-format==15.0.6’

Formatting may be done with something like (following works on Mac):
find -E . -regex ‘.*.(cpp|hpp|c|h)’ -exec clang-format -i {} +

… it fails on Linux, though

find: unknown predicate `-E’)

I was pretty sure this would deviate from Mac/BSD on Linux systems:

Try something like (untested):

find . -regex ‘.*.(cpp|hpp|c|h)’ -exec clang-format -i {} ;

or manually with:
clang-format -I

Contribution rules must be updated, I will start putting up a draft ASAP.

I am trying to fix the conflicts in
https://github.com/OSGeo/grass/pull/2684

Conflicting files:

include/grass/iostream/mm.h
lib/db/dbmi_base/dbmscap.c
lib/external/ccmath/ccmath.h
lib/gis/spawn.c
lib/gis/user_config.c
lib/iostream/rtimer.cpp
lib/pngdriver/graph_set.c
lib/rst/interp_float/point2d.c
raster/r.terraflow/filldepr.cpp
raster/r.terraflow/flow.cpp
raster/r.terraflow/main.cpp
raster/r.viewshed/statusstructure.cpp

The reason will be the missing clang-format update which I don’t know
how to apply on Linux.

Not quite sure what you mean. Did you try:

python -m pip install 'clang-format==15.0.6’

But I suspect any version 15 will do, perhaps even v. 14.

A tip to use the .clang-format file from main for branches without it:

  1. git checkout main
  2. cp .clang-forrmat …/.clang-format
  3. check out branch
  4. clang-format searches upwards in dir hierarchy for next ‘.clang-format’ file
  5. run clang-format from grass source dir
_______________________________________________
grass-dev mailing list
[grass-dev@lists.osgeo.org](mailto:grass-dev@lists.osgeo.org)
[https://lists.osgeo.org/mailman/listinfo/grass-dev](https://lists.osgeo.org/mailman/listinfo/grass-dev)

-- 
[http://www.spatialys.com](http://www.spatialys.com/)
My software is free, but my time generally not.

On 26 Jan 2023, at 15:07, Nicklas Larsson <n_larsson@yahoo.com> wrote:

The question now is, are there any objections to do the same treatment to grass-addons)?
There are three PRs in-store [3-5] to make that happen.

I understand there are no objections to apply clang-format and do the general trailing-whitespace/EOF cleanup in grass-addons.

I will in short proceed and merge the related PRs [3-5].

[1] https://pre-commit.com
[2] https://github.com/OSGeo/grass/pull/2765
[3] https://github.com/OSGeo/grass-addons/pull/852
[4] https://github.com/OSGeo/grass-addons/pull/853
[5] https://github.com/OSGeo/grass-addons/pull/854

Best,
Nicklas