Replacing fixcc.py with clang-format?

[Jean Abou Samra]

Hi,

There's one thing I'd like to discuss now, for the reformatting round
before we do the branching.

With https://gitlab.com/lilypond/lilypond/-/merge_requests/1603,
clang-format uses a style that is more consistent with our existing
style. Although it still changes around 8500 lines, most of this is
folding overlong lines, which fixcc.py does not (always?) do.

I would like to propose moving to clang-format as the canonical
formatting tool and removing fixcc.py. This will decrease our
maintenance burden. fixcc.py is more than 600 lines of code to
maintain. For example, when we agreed to use C++ raw strings for
docstrings, Dan had to modify fixcc.py to avoid breaking them.

Please note that unlike the previous proposal for clang-format [*],
which didn't meet consensus, I am not proposing to require
contributors to run the formatting tool systematically, or to
change anything to the formatting procedure. This is only about
replacing fixcc.py with clang-format, nothing else. We'd continue
to run autoformatting in each release cycle before branching the
next stable release. As far as I can see, the main objections to
the previous proposal were with the principle of applying
formatting on all contributions, and with the size of clang-format.
For a tool that doesn't need to be installed by all contributors
but just by the developer who runs it once every release, size
should not be an issue.

[*] https://lists.gnu.org/archive/html/lilypond-devel/2020-01/msg00684.html

Although maintenance remains the main reason for doing this in
my book, the style decisions made by clang-format seems sensible
to me. In general, it seems to enforce our style more consistently.
Here are some examples that I liked:


-----

-      if (argument[ 1 ])
+      if (argument[1])


-----


-              if (Page_layout_problem::read_spacing_spec (spec,
- &spaceable_min_distance,
- ly_symbol2scm ("minimum-distance")))
-                dy = std::max (dy, spaceable_min_distance + 
stacking_dir * (last_spaceable_element_pos - where));
-
-              dy = std::max (dy, Page_layout_problem::get_fixed_spacing 
(last_spaceable_element, elems[j], spaceable_count,
- pure, start, end));
+              if (Page_layout_problem::read_spacing_spec (
+                    spec, &spaceable_min_distance,
+                    ly_symbol2scm ("minimum-distance")))
+                dy = std::max (
+                  dy, spaceable_min_distance
+                        + stacking_dir * (last_spaceable_element_pos - 
where));
+
+              dy = std::max (dy, Page_layout_problem::get_fixed_spacing (
+                                   last_spaceable_element, elems[j],
+                                   spaceable_count, pure, start, end));
             }

-----


-Real ape_priority (Accidental_placement_entry const *a)
+Real
+ape_priority (Accidental_placement_entry const *a)


-----

-  static SCM automatic_shift (Grob *, Drul_array<std::vector<Grob *> >);
+  static SCM automatic_shift (Grob *, Drul_array<std::vector<Grob *>>);

-----

-class One_page_breaking: public Page_breaking
+class One_page_breaking : public Page_breaking

-----

-# ifdef CHAR
-# define LILY_CHAR CHAR
-# undef CHAR
-# endif
[...]
+#ifdef CHAR
+#define LILY_CHAR CHAR
+#undef CHAR
+#endif
[...]

-----


To try it yourself, check out
https://gitlab.com/lilypond/lilypond/-/merge_requests/1603
and run

clang-format -i $(git ls-files "*.cc" "*.hh" "*.tcc")
git diff

These days, git blame supports the --ignore-rev option, which makes
this less painful for future code historians. It will obviously still
produce merge conflicts with existing WIP branches. I think the reduced
maintenance costs on the long term will offset this one-time cost.

It is also an option to use "ColumnLimit: 0" in the .clang-format config
file. This reduces the diff size from 8500 to 3000 lines by putting
no limit on the line length (fixscm.sh actually doesn't rewrap long
lines either).

Thoughts?

Thanks,
Jean